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

[Dan Liew via Phabricator via lldb-commits]

delcypher added inline comments.


================
Comment at: lldb/source/API/SBWatchpoint.cpp:319
+  }
+  return false;
+}
----------------
@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.


================
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();
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144937