clayborg wrote: > 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.
I do like the SBWatchpointSpec idea as then we don't need any overloads for the different kinds of watchpoints (address, value, etc). One thing I really like about the options/spec approach is if you are writing a new command in lldb that sets a watchpoint, and if that command has options in the python, then it is easy to populate the options/spec gradually and then set a watchpoint at the end. So the flow can be: - Create a default "SBWatchPointSpec spec;" - Parse the options as they come in and call accessors on the "spec" object to setup the watchpoint - Call SBTarget::WatchpointCreate(spec) That being said I understand that it makes using python to quickly set watchpoints a bit more of a pain, so I am not voting 100% in any direction, just throwing stuff out there. I already have to check the APIs everytime I use python since we have many overloads on functions already, not a big deal if we add more. One thing to think about though is to make sure there aren't and default python arguments that would make it hard to call the existing API or the new API. There is some build warning when I build right now about a method being covered up due to default args when swig parses the header files + .i files that might be having this issue. So if we go the route of overloading we just need to make sure we avoid that issue in python. The other way we can configure this is to add an accessor to the SBWatchpoint object itself for non common options like "stop on all writes even when not modified". So we could leave the create API alone, and then add a method like: ``` bool SBWatchpoint::SetStopOnAllWrites(bool b); ``` So we do the right thing by only stopping on a write the modifes by default and then we can further configure the breakpoint after it has been created. Looking at the 6 overloads for setting breakpoints by file and line, 7 for breakpoint by name(s), 6 for regex, I still vote either the options route or adding an accessor on the SBWatchpoint object after it is created in lieu of adding overloads. 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
