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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib 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;
+  }
----------------
JDevlieghere wrote:
> 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?
That sounds fair to me. Let's go with your suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143104