JDevlieghere added a comment.
I'm very excited about this feature. Great job on the documentation, both in
the help output as for the website. Do you have a potential use case in mind
that we could add to the examples?
================
Comment at: lldb/docs/use/python-reference.rst:843
+| | | **target** is
the SBTarget to which the stop hook is added.
|
+| | |
|
+| | |
|
----------------
Any reason for the double empty lines?
================
Comment at: lldb/docs/use/python-reference.rst:863
+
+::
+ (lldb) command script import MyModule.py
----------------
There needs to be a newline here for this to be rendered as code.
================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:302
+ virtual StructuredData::GenericSP
+ CreateScriptedStopHook(lldb::TargetSP target_sp, const char *class_name,
+ StructuredDataImpl *args_data, Status &error) {
----------------
Could this function return an `Expected<StructuredData::GenericSP>` instead?
================
Comment at: lldb/include/lldb/Target/Target.h:1198
+
+ StringList *GetCommandPointer() { return &m_commands; }
+ const StringList &GetCommands() { return m_commands; }
----------------
Can we drop this altogether? We can always get the address of the reference at
the call site.
================
Comment at: lldb/include/lldb/Target/Target.h:1200
+ const StringList &GetCommands() { return m_commands; }
+ void SetActionFromString(std::string &strings);
+ void SetActionFromStrings(std::vector<std::string> &strings);
----------------
can this be a const reference? Maybe even a `StringRef`? Similar question for
the methods below.
================
Comment at: lldb/include/lldb/Target/Target.h:1231
+ std::string m_class_name;
+ StructuredDataImpl *m_extra_args; // We own this structured data,
+ // but the SD itself manages the UP.
----------------
Please make these Doxygen comments (`///`) and put them on the line above the
variable.
================
Comment at: lldb/include/lldb/Target/Target.h:1237
+ // Use CreateStopHook to make a new empty stop hook. The GetCommandPointer
+ // and fill it with commands, and SetSpecifier to set the specifier shared
+ // pointer (can be null, that will match anything.)
----------------
`///`
================
Comment at: lldb/include/lldb/Target/Target.h:1246
// Add an empty stop hook to the Target's stop hook list, and returns a
// shared pointer to it in new_hook. Returns the id of the new hook.
----------------
And you might as well fix that here too :-)
================
Comment at: lldb/include/lldb/Target/Target.h:1248
// shared pointer to it in new_hook. Returns the id of the new hook.
- StopHookSP CreateStopHook();
+ StopHookSP CreateStopHook(bool use_commands);
+
----------------
I think we should make this a little enum that has the values command and
scripted.
================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4691
+ // The IOHandler editor is only for command lines stop hooks:
+ Status error;
+ Target::StopHookCommandLine *hook_ptr =
----------------
Is this used?
================
Comment at: lldb/source/Target/Target.cpp:3149
+ SymbolContextSpecifier *specifier = GetSpecifier();
+ if (!specifier)
+ return true;
----------------
So many early returns in this patch 😍
================
Comment at: lldb/source/Target/Target.cpp:3269
+
+ m_extra_args = new StructuredDataImpl();
+
----------------
It looks like we will leak this if script_interp is null. One solution for that
would be to wrap it in a unique_ptr and release it when passing it to
CreateScriptedStopHook. If we do the `m_extra_args` assignment just before that
call that also guarantees that the variable remains null until then.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88123/new/
https://reviews.llvm.org/D88123
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
