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