[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1477-1479
+    bool only_target_values;
+    bool do_clear;
+    bool dummy;
----------------
JDevlieghere wrote:
> jingham wrote:
> > JDevlieghere wrote:
> > > Let's initialize these to the same values as `Clear`.
> > I generally don't initialize the Option ivars on construction, on the grounds that it will mislead people into thinking the initialized values actually matter, which they don't.  They are never used nor should they be.  You always have to call OptionParsingStarting before reading in values for the Options, and you have to reset all the variables there.
> > 
> > But if it bugs you, I can add it.
> I vaguely remember at least one bug where we were using an uninitialized value, but maybe the problem there was that we didn't call `OptionParsingStarting`. If it's possible to forget to call that, then we should have an assert enforce that, but that's beyond the scope of this patch. 
OptionParsingStarting is called for the CommandObjects by the Command runner, so it shouldn't be possible to have that not happen.  But this is indeed a requirement, so it would be worthwhile putting in some kind of assert for this if it's possible.

But the easier mistake to make (and one of the reasons why I'm still a little in favor of not initializing) is to initialize the variable when you add it to Options, but forget to add it to OptionsParsingStarting.  Then you end up getting stale values, and since some of these can be object pointers, you can crash from that.

But I don't know how you would write an assert (short of source analysis) that "all ivars of this class must get reset in method X"...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126259/new/

https://reviews.llvm.org/D126259