[Lldb-commits] [PATCH] D88123: Add the ability to write 'target stop-hooks' in Python

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 9 inline comments as done.
jingham added a comment.

Addressed review comments.



================
Comment at: lldb/docs/use/python-reference.rst:843
+|                    |                                       | **target** is the SBTarget to which the stop hook is added.                                                      |
+|                    |                                       |                                                                                                                  |
+|                    |                                       |                                                                                                                  |
----------------
JDevlieghere wrote:
> Any reason for the double empty lines?
Nope.


================
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) {
----------------
JDevlieghere wrote:
> Could this function return an `Expected<StructuredData::GenericSP>` instead?
There are a bunch of instances of objects created to insert scripting into various lldb callbacks around in lldb.  It might make sense to convert them all to Expected's, but I wouldn't want to do it piecemeal.  

Adding a new one of these is also a bit cargo-culty (another issue we should probably clean up as a separate bit of work) and so making the same things look different is not doing the next person who adds one any favors.

These are also functions that are not going to get called promiscuously, but really from one "make me one of these" places, so they aren't crying out for protection against calling them w/o checking for errors.

But anyway, IMO if we're going to start restructuring the pattern for setting these callback objects up, we should do that as a separate commit and do it for all of them.


================
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);
----------------
JDevlieghere wrote:
> can this be a const reference? Maybe even a `StringRef`? Similar question for the methods below.
const is fine.  Since all I plan to do is pass this string to a function that takes a const std::string, it doesn't make much sense to make it a StringRef.


================
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.
----------------
JDevlieghere wrote:
> Please make these Doxygen comments (`///`) and put them on the line above the variable. 
I didn't quite do that.  

The comment about "We own..." doesn't seem to me to be a doc comment.  It isn't something that a user of this stop hook class would need to know.  It's just an answer to a question somebody reading the code closely might ask about why we don't have to have something keeping this m_extra_args alive.  I did add a doc comment for this field, but kept the side comment as is.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4691
+        // The IOHandler editor is only for command lines stop hooks:
+        Status error;
+        Target::StopHookCommandLine *hook_ptr =
----------------
JDevlieghere wrote:
> Is this used?
Nope.  Vestigial from an older draft of the StopHook API's.


================
Comment at: lldb/source/Target/Target.cpp:3269
+
+  m_extra_args = new StructuredDataImpl();
+
----------------
JDevlieghere wrote:
> 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.
I did this more simple-mindedly by just checking for the script interpreter before we do any other work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88123