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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Utility/ProcessInfo.h:90-110
+  bool IsScriptedProcess() const {
+    return !m_scripted_process_class_name.empty();
+  }
+
+  std::string GetScriptedProcessClassName() const {
+    return m_scripted_process_class_name;
+  }
----------------
mib wrote:
> JDevlieghere wrote:
> > What's the relationship between this an the `ScriptedMetadata`? It seems like this is storing the same stuff. Also `IsScriptedProcess` is a pretty blatant violation of the scripted process being a plugin... 
> I think you have a misunderstanding here: This is addition is not made in a process plugin, but rather in the `ProcessInfo` class, the base class for `ProcessLaunchInfo` and `ProcessAttachInfo`, so I don't think this violates in any way the plugin architecture.
> 
> Regarding the relationship with `ScriptedMetadata`: I guess we could replace `m_scripted_process_class_name` & `m_scripted_process_dictionary_sp` by a `m_scripted_metadata` object, but I didn't want to process info hold scripted process specific stuff. I don't have a strong opinion against it, either.
> I think you have a misunderstanding here: This is addition is not made in a process plugin, but rather in the ProcessInfo class, the base class for ProcessLaunchInfo and ProcessAttachInfo, so I don't think this violates in any way the plugin architecture.

I know, but I would argue that the distinction between those two is pretty much irrelevant. That said, I do understand the need to pass this information, especially as more things become more scriptable (process, platform, etc). That's why I suggested using the `ScriptedMetadata` here because it's more generic.

Concretely my suggestion would be to have `{Set,Get}ScriptedMetadata` and if you think you really need it, maybe an `IsScripted` method that checks if the metadata is set (either by storing an optional ScriptedMetadata in the `ProcessInfo` or maybe through calling an `operator bool` on the `ScriptedMetadata` (which would check the name being empty like you do here). 

> but I didn't want to process info hold scripted process specific stuff. 

I totally agree, as of now `ScriptedMetadata` contains the exact two fields that you have here. If other patches are extending that for the scripted processes or platforms, we could keep the common stuff in a base class and have a ScriptedProcessMetadata and ScriptedPlatformMetadata respectively. WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143104/new/

https://reviews.llvm.org/D143104