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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added inline comments.
This revision is now accepted and ready to land.


================
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) {
----------------
jingham wrote:
> 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.
If I counted correctly there are 2 others: `CreateScriptedThreadPlan` which takes a `std::string&` as an error and `CreateScriptedBreakpointResolver` which doesn't seem to do error handling at all. So I think we should try to refactor all three to return `Expected`s. I agree we should do that in a separate patch. 


================
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.
----------------
jingham wrote:
> 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.
It's private anyway, so I think the boundaries are pretty fluid. But I don't feel strongly about it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88123