jasonmolenda wrote:

> > > ```
> > >    SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address, 
> > > size_t size, uint32_t access_flags, SBError &error);
> > > ```
> > > 
> > > 
> > >     
> > >       
> > >     
> > > 
> > >       
> > >     
> > > 
> > >     
> > >   
> > > with `eWatchpointAccess{Read,Write,Modify}` flags defined.
> > 
> > 
> > @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.

https://github.com/llvm/llvm-project/pull/66308
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to