delcypher marked an inline comment as done.
delcypher added inline comments.
================
Comment at: lldb/source/API/SBWatchpoint.cpp:319
+ }
+ return false;
+}
----------------
bulbazord wrote:
> delcypher wrote:
> > @bulbazord What I don't like about my implementation here is that the user
> > can't tell the difference between
> >
> > 1. there's no shared pointer
> > AND
> > 2. watchpoint is an expression watchpoint.
> >
> > Is 1, common? Feels like this function should really return a
> > `std:optional<bool>` but I'm not sure how to make that play well with SWIG.
> I'm not sure if 1 is common. To distinguish between 1 and 2, clients will
> also want to also check the validity of the watchpoint with `IsValid` or
> `operator bool` since those just get the shared pointer and see if its valid.
> We could make a std::optional work through a swig type map but I'm not sure I
> like the idea of a yes-or-no-question API being able to return 'None'. If
> it's not a watch variable, then 'false' makes sense for an invalid watchpoint.
>
> Another option could be to change the interface a bit. Maybe create an enum
> called WatchpointKind containing 3 values, "Variable", "Expression", and
> "Invalid". You could then change this method to be `GetWatchpointKind` and
> return the right value depending on a combination of `GetSP()` and
> `IsWatchVariable`.
Done. I ended up giving it a different name because I discovered there's
already a `WatchpointKind` enum and I thought it best to not confuse the two.
================
Comment at: lldb/source/API/SBWatchpoint.cpp:329-341
+ // We can't return `watchpoint_sp->GetWatchSpec().c_str()`
+ // because the temporary std::string will be destroyed
+ // when this function finishes. Instead we store our own
+ // copy in this class and give clients the C string used
+ // by the copy.
+ if (m_cached_watch_spec.size() == 0) {
+ m_cached_watch_spec = watchpoint_sp->GetWatchSpec();
----------------
delcypher wrote:
> JDevlieghere wrote:
> > As Ismail and Jason pointed out, the way to do this is wrapping it into a
> > ConstString:
> >
> > ```
> > return ConstString(watchpoint_sp->GetWatchSpec()).AsCString();
> > ```
> Sure. Based on `ConstString`'s documentation looks like it uses a pool of
> strings that lives for ever. So we'll be leaking memory here. I guess that's
> acceptable because this method is unlikely to be called often, and even if it
> is we only waste memory per unique watchpoint spec.
>
> This sounds a lot simpler than implementing a `WatchpointImpl`.
Done.
================
Comment at: lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py:66
+ if variable_watchpoint:
+ # FIXME: There should probably be an API to create a variable
watchpoint
+ self.runCmd('watchpoint set variable -w read_write -- global')
----------------
delcypher wrote:
> mib wrote:
> > +1, please file an enhancement request :)
> Will do. TBH I'm actually surprised that calling `SBValue::Watch()` doesn't
> create a variable watchpoint.
Actually I'm even more confident this is a bug. If `value.Watch(...)` created
an expression watchpoint you would expect the type to be a `int32_t*` (because
the watched expression would need to have been `&global`), not `int32_t`.
However, I think this bug should be fixed in a different commit.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144937/new/
https://reviews.llvm.org/D144937
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
