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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
