jimingham wrote:
I'm not sure I see the benefit of the SBWatchpointSpec including the way the
watched region is specified. That's just moving the proliferation of API's
from SBTarget::WatchpointCreate*() to the SBWatchpointSpec constructor. We'd
have to have:
SBWatchpointSpec::SBWatchpointSpec(SBAddress addr, int flags);
SBWatchpointSpec::SBWatchpointSpec(SBValue value, int flags);
SBWatchpointSpec::SBWatchpointSpec(const char *expr, int flags);
And then users would do:
wp_spec = lldb.SBWatchpointSpec("foo + 5", lldb.eWatchpointTypeModify)
wp = target.WatchpointCreate(wp_spec)
So that's really one more API plus some boilerplate to construct the options.
If an SBWatchpointSpec were the sort of thing you'd reuse, there might be some
sense in making that a separate entity, but I don't see that for watchpoints.
Otherwise, I don't think it adds much.
Jim
> On Sep 14, 2023, at 3:12 PM, Jason Molenda ***@***.***> wrote:
>
>
> SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address, size_t
> size, uint32_t access_flags, SBError &error);
> with eWatchpointAccess{Read,Write,Modify} flags defined.
>
> @bulbazord <https://github.com/bulbazord> what do you think about this
> suggestion? Would you still prefer an Options class?
>
> If we are going to add an overload I would suggest keeping with just adding a
> "bool modify" as it is more clear an usable. The options does seem like
> overkill for just one bool I admit, it just depends on what kind of options
> we might want to watchpoints in the future. If this is the last change to
> watchpoints, then add a new API. If we think we will add more options at some
> point (try to think if the new fancy watchpoints Jason is about to add
> support for might need more options?) then do the Options class route.
>
> You and Alex both preferred adding an Options class to pass in to this (and
> future WatchpointCreate API) so I'll write that up for my next revision of
> this PR, I didn't see your earlier message talking about your preference for
> that when I ping'ed Alex on their opinion.
>
> You were talking about how you think the proliferation of BreakpointCreate
> API was not ideal. Do you think there should be a
> SBTarget::WatchpointCreate(SBWatchpointSpec, SBError &error) and you would
> specify (1) addr + size, (2) variable, (3) expression + size, as well as the
> access flags, for the Watchpoint? I don't mind the separate SBTarget methods
> approach for Breakpoints, it seems like six of one, half a dozen of the other
> to me.
>
> It's probably best to just go with SBTarget::WatchpointCreateByAddress and
> add a new SBWatchpointOptions with the access flags only.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/llvm/llvm-project/pull/66308#issuecomment-1720222526>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ADUPVW4XRSYKJBWO3X4RAY3X2N6NVANCNFSM6AAAAAA4XKAUOQ>.
> You are receiving this because your review was requested.
>
https://github.com/llvm/llvm-project/pull/66308
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
