[cfe-commits] [PATCH] Checking for sometimes-uninitialized variables

[David Blaikie]

>> If CFG iterators are so flexible as to allow array access, perhaps we
>> should just add an op[] to the CFG type itself rather than indexing
>> into an iterator?
>
> Good idea, I meant to come back to that, but forgot. Thanks! I've added an
> operator[] to CFG.

& it returns a reference, awesome :) (I'd noticed the previous version
of the while loop had a local CFGBlock pointer that it just
dereferenced multiple times, but figured it wasn't worth mentioning
given the prevalence of pointers that pervades the code base)

>> > #if 0
>> >     cfg.dump(dc.getParentASTContext().getLangOpts(), true);
>> > #endif
>>
>> Not sure if you meant to leave this in, nor whether it should be
>> preprocessed out, constant conditional-ed out, or commented out, if it
>> is to remain (I sort of like the constant conditional option - that
>> ensures that it doesn't bitrot. Between preprocessing and commenting
>> out, I'd probably choose commenting out.
>
>
> This file has a pile of "#if 0"s for debugging already, which produce all
> the useful information except for dumping the CFG. This seemed like a
> natural addition (though it would be great to be able to enable these
> without a rebuild...).

ah, fair enough. If it's likely they'll all be turned on/off, they
could at least go under one macro definition at the top of the file -
but I certainly wouldn't hold you to do that just because you're
adding one more.

>> > switch (Use.getKind()) {
>> > case UninitUse::Always: DiagID = diag::warn_uninit_var; break;
>> > case UninitUse::Sometimes: DiagID = diag::warn_sometimes_uninit_var;
>> > break;
>> > case UninitUse::Maybe: DiagID = diag::warn_maybe_uninit_var; break;
>>
>> OCD: worth aligning the DiagID so the maybe/sometimes/ stand out better?
>>
>> The switch 10 lines down is also written with multiple line breaks
>> (couldn't quite fit it under 80 cols, I guess?) Perhaps worth just
>> moving the switch out to the top & doing it unconditionally?
>
>
> The two switches actually produce different values.

Ah.

>> > // FIXME: This iteration order, and thus the resulting diagnostic order,
>> > //        is nondeterministic.
>>
>> Is it just luck that we don't test the cases where this would come up?
>> Or are our tests unstable & just haven't exhibited it yet?
>
> -verify doesn't care which order diagnostics appear in.

Oh, right.

> IIRC, at least one of current tests exhibits nondeterminism.

Pity - wonder if there's anything we should add to the testing
infrastructure to help with this sort of thing in general.

>> > - if (DiagnoseUninitializedUse(S, vd, vi->first,
>> > -                              /*isAlwaysUninit=*/vi->second))
>> > - // If we have self-init, downgrade all uses to 'may be uninitialized'.
>> > + UninitUse Use = hasSelfInit ? UninitUse(vi->getUser(), false) : *vi;
>> > + if (DiagnoseUninitializedUse(S, vd, Use))
>>
>> Is this a change in behavior, or is the comment clarifying existing
>> behavior? (just a little curious because the 'isAlwaysUninit'
>> parameter went from 'vi->second' to 'false')
>
>
> There's no change in behavior. If there's a self-init and we have an
> always-uninitialized use, we can't get here (the diff cuts off the relevant
> context, which is just a few lines above).

OK

>> > static void NoteUninitBranches(Sema &S, const UninitUse &Use) {
>>
>> Should you use a switch in the diagnostic rather than string literals
>> in the source for the 'for', 'if' , ':?', etc? (I realize those are
>> keywords & can't be translated, which is the usual hypothetical
>> argument for these things - so it's not a definite
>> improvement/important change in that regard)
>
>
> I started that way, but it led to a very repetitive definition in the
> diagnostic.

Fair enough.

>>
>> Diagnostic text: the note trace talks about loops being "entered" but
>> then about them being "terminated", would it make sense to refer to
>> them as "exited" and/or is there precedent for any other terminology
>> here? (& I still haven't come up with any better way to describe the
>> "loop exited through its condition becoming false" than your
>> "exited/terminated 'normally'")
>
>
> Yes, that not isn't as clear as it could be. I've changed this to
> "uninitialized use occurs whenever 'for' loop exits because its condition is
> false".

Sounds good!

>> The shortcircuit cases are nice - does the note point to the && now, I
>> assume? (you could split the expressions over multiple lines to
>> demonstrate this) Is it worth any clarity to point to the first
>> operand to the &&? I'm wondering if people would be confused if they
>> saw:
>
>
> The diagnostic points to the expression on the LHS of the && (the one whose
> value is being described in the note). I've updated the tests as you
> suggest.
>
>>
>> if (x && y) { // whenever && condition is false
>
>
> In this case, we say 'whenever the 'if' condition is false'.

Sorry, I didn't explain this well. I meant to provide an example where
the second condition to the && initializes the variable:

int y;
if (x && (y = z)) // expected-warning {{whenever && condition is false}}

When a user sees this for the first time, they might misunderstand the
"&& condition" as referring to the whole condition, not the first part
of it (because they've never seen the "whenever 'if' condition is
false" diagnostic text to realize that "&& condition" isn't referring
to the whole condition, but just the first part).

But with the actual example written above, the variable initialization
is clearly visible so perhaps the confusion isn't very
likely/realistic.

Thanks,
- David