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

delcypher created this revision. Herald added a project: All. delcypher requested review of this revision. Herald added a project: LLDB. This patch adds the following methods: - `GetType()` - `IsWatchVariable()` - `GetWatchSpec()` These effectively expose methods that `lldb_private::Watchpoint`

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

delcypher updated this revision to Diff 500993. delcypher added a comment. Herald added a subscriber: JDevlieghere. Fix reviewers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144937/new/ https://reviews.llvm.org/D144937 Files: lldb/bindings/int

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

delcypher updated this revision to Diff 500994. delcypher added a comment. Fix typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144937/new/ https://reviews.llvm.org/D144937 Files: lldb/bindings/interface/SBWatchpointDocstrings.i lldb/include

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

mib added a comment. Most of this looks fine to me, besides the WatchSpec cache string in the SB class. I don't think the spec changes after the creation of the watchpoint, so may be this WatchSpec object should be a `lldb_private::ConstString` and from there you'll be able to get your `c_strin

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

mib added inline comments. 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_wri

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

mib added inline comments. Comment at: lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py:83 +self.assertFalse(watchpoint.IsWatchVariable()) +self.assertEqual(watchpoint.GetWatchSpec(), '') + mib wrote: > unrelated to this patch, the

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

jasonmolenda added a comment. Hi Dan, I hadn't looked this over very closely but one thing that jumped out is that you're adding a member to SBWatchpoint, and we can't do that, it's an API breaking change. In cases where we need to store additional information than a weak pointer to an lldb pr

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

jasonmolenda added a comment. In D144937#4159324 , @jasonmolenda wrote: > In cases where we need to store additional information than a weak pointer to > an lldb private object, we traditionally add an Impl class which has the > additional member(s) an

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

JDevlieghere requested changes to this revision. JDevlieghere added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/API/SBWatchpoint.cpp:329-341 +// We can't return `watchpoint_sp->GetWatchSpec().c_str()` +// because the temporary s

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

delcypher added a comment. In D144937#4159324 , @jasonmolenda wrote: > Hi Dan, I hadn't looked this over very closely but one thing that jumped out > is that you're adding a member to SBWatchpoint, and we can't do that, it's an > API breaking change.

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

bulbazord added a comment. Seems like you've already got feedback about breaking ABI. Small nit: Comment at: lldb/bindings/interface/SBWatchpointDocstrings.i:30-31 +%feature("docstring", " +Returns true if the watchpoint is a variable watchpoint. Otherwise the +watchpoi

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

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. watchpoin

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

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

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

delcypher added inline comments. 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 re

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

mib added inline comments. Comment at: lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py:68 +self.runCmd('watchpoint set variable -w read_write -- global') +watchpoint = target.GetWatchpointAtIndex(0) +self.assertTrue(watchpoint.IsWatchV

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

delcypher updated this revision to Diff 501356. delcypher added a comment. - Add `WatchpointValueKind` enum - Use `WatchpointValueKind` for new `GetWatchValueKind()` method (previously named `SBWatchpoint::IsWatchVariable`) - Add `IsWatchingsRead()` method - Add `IsWatchingWrites()` method - Remo

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

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 us

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

delcypher marked an inline comment as done. delcypher added a comment. @JDevlieghere @mib @bulbazord Thanks for all the feedback. This patch is ready for another review pass. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144937/new/ https://review

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

mib accepted this revision. mib added a comment. LGTM! Thanks for your patch @delcypher Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144937/new/ https://reviews.llvm.org/D144937 ___ lldb-commits mailing

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

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM with two small nits. Comment at: lldb/include/lldb/API/SBWatchpoint.h:14 +#include "lldb/API/SBType.h" +#include No longer necessary? =

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

delcypher updated this revision to Diff 501582. delcypher edited the summary of this revision. delcypher added a comment. - Fix nits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144937/new/ https://reviews.llvm.org/D144937 Files: lldb/bindings/

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

delcypher marked 2 inline comments as done. delcypher added a comment. Thanks for the review and approval. I've the fixed the nits so I'm going to land. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144937/new/ https://reviews.llvm.org/D144937 __

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG55a363fea18b: [LLDB] Expose several methods in SBWatchpoint (authored by delcypher). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144937/new/ https://revie

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

jingham added inline comments. 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