JDevlieghere accepted this revision as: JDevlieghere.
JDevlieghere added a comment.
This revision is now accepted and ready to land.
A few small nits, but otherwise this LGTM.
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointOptions.h:120
+ ///
BreakpointOptions(const char *condition, bool enabled = true,
int32_t ignore = 0, bool one_shot = false,
----------------
shafik wrote:
> mib wrote:
> > shafik wrote:
> > > You have a lot of `bool` parameters, these are hard to distinguish when
> > > calling the function and easy to get mixed up during refactors and
> > > subsequent merge conflicts. It would probably be better to combine these
> > > `bool` options into a `struct` and then each option has an explicit name
> > > that that will be assigned to which makes it explicit which options are
> > > being chosen at the call site.
> > I only added the `bool inject_condition` parameter to the
> > `BreakpointOptions` constructor. I also added documentation that was
> > missing for the other parameters. I don't think having a struct for those
> > options is a necessary since that's what the `BreakpointOptions` class is
> > for.
> Sadly we won't have designated initializers until C++20 [see
> godbolt](https://godbolt.org/z/2xlti5) but instead of `bool` we can use enums
> and that would clarify the code at the calling site. As discussed this can be
> done as a patch after landing this change,
I agree with Shafik, but on the other hand that's the way the API is today. I
don't really like the idea of a struct, because that just hides the booleans.
Maybe we can use the OptionKind enum directly? Regardless, as you said, that's
definitely outside the scope of this patch.
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointOptions.h:364
+ void SetInjectCondition(bool inject_condition) {
+ m_inject_condition = inject_condition;
+ m_set_flags.Set(eInjectCondition);
----------------
Any reason this should be in the header?
================
Comment at: lldb/source/Breakpoint/BreakpointOptions.cpp:113
+ "ConditionText", "IgnoreCount", "EnabledState",
+ "OneShotState", "AutoContinue", "JITCondition"};
----------------
Inject(ed)Condition?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66248/new/
https://reviews.llvm.org/D66248
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
