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