bulbazord added a comment.
I'm not sure how much this matters but the fact that
`ScriptedMetadata::IsValid` (and the bool operator overload) relies on the
class name being empty is a little strange to me. I feel like you could get
yourself into a strange situation where you set the ScriptedMetadata class name
to "" and then ask for it right back and instead of giving you "" it'll give
you nullptr (or None in python). This also "invalidates" the ScriptedMetadata.
Not a hard blocker, just a little strange to me.
================
Comment at: lldb/source/API/SBAttachInfo.cpp:273-278
+ ScriptedMetadataSP metadata_sp = m_opaque_sp->GetScriptedMetadata();
- m_opaque_sp->SetScriptedProcessClassName(class_name);
+ if (!metadata_sp)
+ metadata_sp = std::make_shared<ScriptedMetadata>(class_name, nullptr);
+
+ m_opaque_sp->SetScriptedMetadata(metadata_sp);
----------------
Maybe I'm misunderstanding but it seems like this implementation doesn't allow
you to actually change the class name more than once? If that's not
intentional, then we should be creating a new ScriptedMetadata and calling
`SetScriptedMetadata` every time. If it is intentional, why?
================
Comment at: lldb/source/API/SBAttachInfo.cpp:312-317
+ ScriptedMetadataSP metadata_sp = m_opaque_sp->GetScriptedMetadata();
+
+ if (!metadata_sp)
+ metadata_sp = std::make_shared<ScriptedMetadata>("", dict_sp);
+
+ m_opaque_sp->SetScriptedMetadata(metadata_sp);
----------------
Same as above: This doesn't actually let you change the arguments if we already
have an existing ScriptedMetadata. If its already set, we're just calling
`SetScriptedMetadata` with what already exists and discarding the argument.
================
Comment at: lldb/source/API/SBAttachInfo.cpp:272
void SBAttachInfo::SetScriptedProcessClassName(const char *class_name) {
LLDB_INSTRUMENT_VA(this, class_name);
+ ScriptedMetadataSP metadata_sp = m_opaque_sp->GetScriptedMetadata();
----------------
JDevlieghere wrote:
> Newline after `LLDB_INSTRUMENT_VA`
There needs to be a newline after `LLDB_INSTRUMENT_VA` here I believe.
================
Comment at: lldb/source/API/SBLaunchInfo.cpp:348
LLDB_INSTRUMENT_VA(this, class_name);
-
- m_opaque_sp->SetScriptedProcessClassName(class_name);
+ ScriptedMetadataSP metadata_sp = m_opaque_sp->GetScriptedMetadata();
+ StructuredData::DictionarySP dict_sp =
----------------
Needs to be a newline after `LLDB_INSTRUMENT_VA`
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:203-208
+ SetPrivateState(eStateRunning);
+
+ if (error.Fail())
+ return error;
+
+ SetPrivateState(eStateStopped);
----------------
I'm not sure I understand what the point of setting the state to `Running` is
and only setting it to `Stopped` if the attach failed? Should we be mucking
with state at all if the attach failed?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143104/new/
https://reviews.llvm.org/D143104
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
