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

[David Blaikie]

On Thu, May 17, 2012 at 1:01 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Hi,
>
> The attached patch adds a new classification of uninitialized variables,
> between 'always used uninitialized' and 'may be used uninitialized', which
> (unlike 'may be used uninitialized') has no false positives (after making
> one conservative assumption), and produces more warnings than 'always used
> uninitialized'.

The principle and results look great. Here's some nitpicky/code review
details, though:

> include/clang/Analysis/Analyses/UninitializedValues.h:
> /// If non-NULL, this use is always uninitialized if occurs after any of

If what is non-NULL (ultra-pedantry/pet-peeve: pointer values are
null, not NULL - the latter being just a specific constant)? The
vector? "non-empty", perhaps?

"if occurs" -> "if it occurs" ?

> /// these branches is taken.

"any of these branches are taken" ? It sounds more right, but strictly
speaking it feels wrong because you're talking about only one of
them... I'm not sure what the correct grammatical rules are here, it
just sounded weird when I read it.

> const CFGBlock *B = cfg.begin()[Queue.back()];

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?

> #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.

> 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?

> // 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?

> - 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')

> 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)

>      // "condition is true / condition is false".
>    case Stmt::IfStmtClass:
...
>      // "loop is entered / loop is exited".
>    case Stmt::WhileStmtClass:
...
>      // "switch case is taken".
>    case Stmt::CaseStmtClass:

Alignment of these comments is a bit strange (they're indented, but
they pertain (as far as I understand it) to the following case which
is not indentend). It might even be better on the same line as (&
following) the DiagKind assignment. Hmm, I see - it pertains to a set
of cases, yeah, maybe just unindenting would suffice - but I can see
that it's not clear either way how to group the case statements under
the comment.

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'")

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:

if (x && y) { // whenever && condition is false

and think it was talking about the condition for the if block, rather
than that of the 'x' - but perhaps it's good enough that we get that
far.

- David