<div class="gmail_quote">On Thu, May 17, 2012 at 2:04 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Thu, May 17, 2012 at 1:01 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> Hi,<br>
><br>
> The attached patch adds a new classification of uninitialized variables,<br>
> between 'always used uninitialized' and 'may be used uninitialized', which<br>
> (unlike 'may be used uninitialized') has no false positives (after making<br>
> one conservative assumption), and produces more warnings than 'always used<br>
> uninitialized'.<br>
<br>
</div>The principle and results look great. Here's some nitpicky/code review<br>
details, though:<br>
<br>
> include/clang/Analysis/Analyses/UninitializedValues.h:<br>
> /// If non-NULL, this use is always uninitialized if occurs after any of<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

If what is non-NULL (ultra-pedantry/pet-peeve: pointer values are<br>
null, not NULL - the latter being just a specific constant)? The<br>
vector? "non-empty", perhaps?<br>
<br>
"if occurs" -> "if it occurs" ?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> /// these branches is taken.<br>
<br>
"any of these branches are taken" ? It sounds more right, but strictly<br>
speaking it feels wrong because you're talking about only one of<br>
them... I'm not sure what the correct grammatical rules are here, it<br>
just sounded weird when I read it.<br></blockquote><div><br></div><div>I was taught that "{none,one,neither,either,any} of these things" is a singular, since we're referring to just one of them at a time. But I would not be surprised if that isn't universal amongst English-speakers.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> const CFGBlock *B = cfg.begin()[Queue.back()];<br>
<br>
If CFG iterators are so flexible as to allow array access, perhaps we<br>
should just add an op[] to the CFG type itself rather than indexing<br>
into an iterator?<br></blockquote><div><br></div><div>Good idea, I meant to come back to that, but forgot. Thanks! I've added an operator[] to CFG.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

> #if 0<br>
>     cfg.dump(dc.getParentASTContext().getLangOpts(), true);<br>
> #endif<br>
<br>
Not sure if you meant to leave this in, nor whether it should be<br>
preprocessed out, constant conditional-ed out, or commented out, if it<br>
is to remain (I sort of like the constant conditional option - that<br>
ensures that it doesn't bitrot. Between preprocessing and commenting<br>
out, I'd probably choose commenting out.<br></blockquote><div><br></div><div>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...).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> switch (Use.getKind()) {<br>
> case UninitUse::Always: DiagID = diag::warn_uninit_var; break;<br>
> case UninitUse::Sometimes: DiagID = diag::warn_sometimes_uninit_var; break;<br>
> case UninitUse::Maybe: DiagID = diag::warn_maybe_uninit_var; break;<br>
<br>
OCD: worth aligning the DiagID so the maybe/sometimes/ stand out better?<br>
<br>
The switch 10 lines down is also written with multiple line breaks<br>
(couldn't quite fit it under 80 cols, I guess?) Perhaps worth just<br>
moving the switch out to the top & doing it unconditionally?<br></blockquote><div><br></div><div>The two switches actually produce different values.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

> // FIXME: This iteration order, and thus the resulting diagnostic order,<br>
> //        is nondeterministic.<br>
<br>
Is it just luck that we don't test the cases where this would come up?<br>
Or are our tests unstable & just haven't exhibited it yet?<br></blockquote><div><br></div><div>-verify doesn't care which order diagnostics appear in. IIRC, at least one of current tests exhibits nondeterminism.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> - if (DiagnoseUninitializedUse(S, vd, vi->first,<br>
> -                              /*isAlwaysUninit=*/vi->second))<br>
> - // If we have self-init, downgrade all uses to 'may be uninitialized'.<br>
> + UninitUse Use = hasSelfInit ? UninitUse(vi->getUser(), false) : *vi;<br>
> + if (DiagnoseUninitializedUse(S, vd, Use))<br>
<br>
Is this a change in behavior, or is the comment clarifying existing<br>
behavior? (just a little curious because the 'isAlwaysUninit'<br>
parameter went from 'vi->second' to 'false')<br></blockquote><div><br></div><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> static void NoteUninitBranches(Sema &S, const UninitUse &Use) {<br>
<br>
Should you use a switch in the diagnostic rather than string literals<br>
in the source for the 'for', 'if' , ':?', etc? (I realize those are<br>
keywords & can't be translated, which is the usual hypothetical<br>
argument for these things - so it's not a definite<br>
improvement/important change in that regard)<br></blockquote><div><br></div><div>I started that way, but it led to a very repetitive definition in the diagnostic.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

>      // "condition is true / condition is false".<br>
>    case Stmt::IfStmtClass:<br>
...<br>
>      // "loop is entered / loop is exited".<br>
>    case Stmt::WhileStmtClass:<br>
...<br>
>      // "switch case is taken".<br>
>    case Stmt::CaseStmtClass:<br>
<br>
Alignment of these comments is a bit strange (they're indented, but<br>
they pertain (as far as I understand it) to the following case which<br>
is not indentend). It might even be better on the same line as (&<br>
following) the DiagKind assignment. Hmm, I see - it pertains to a set<br>
of cases, yeah, maybe just unindenting would suffice - but I can see<br>
that it's not clear either way how to group the case statements under<br>
the comment.<br></blockquote><div><br></div><div>I've unindented the comments. I think you're right that that's a little better.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Diagnostic text: the note trace talks about loops being "entered" but<br>
then about them being "terminated", would it make sense to refer to<br>
them as "exited" and/or is there precedent for any other terminology<br>
here? (& I still haven't come up with any better way to describe the<br>
"loop exited through its condition becoming false" than your<br>
"exited/terminated 'normally'")<br></blockquote><div><br></div><div>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".</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The shortcircuit cases are nice - does the note point to the && now, I<br>
assume? (you could split the expressions over multiple lines to<br>
demonstrate this) Is it worth any clarity to point to the first<br>
operand to the &&? I'm wondering if people would be confused if they<br>
saw:<br></blockquote><div><br></div><div>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.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
if (x && y) { // whenever && condition is false<br></blockquote><div><br></div><div>In this case, we say 'whenever the 'if' condition is false'.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

and think it was talking about the condition for the if block, rather<br>
than that of the 'x' - but perhaps it's good enough that we get that<br>
far.</blockquote><div><br></div><div>I think we'd only want to get the '&&' diagnostic here if the initialization happens within 'y' itself (though we don't currently catch that case at all, because the uninitialized variable tracking code's special case for '&&' and '||' doesn't quite do the right thing when they're immediately used in a condition).</div>
<div><br></div><div>Updated patch attached.</div><div><br></div><div>-- Richard</div></div>