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