[Lldb-commits] [PATCH] D144937: [LLDB] Expose several methods in SBWatchpoint

[Dan Liew via Phabricator via lldb-commits]

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