[Lldb-commits] [PATCH] D143104: [lldb/Plugins] Add Attach capabilities to ScriptedProcess

[Alex Langford via Phabricator via lldb-commits]

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