[cfe-commits] r160330 - in /cfe/trunk: lib/Analysis/UninitializedValues.cpp test/Sema/uninit-variables.c test/SemaCXX/uninit-variables.cpp

[Richard Smith]

On Mon, Jul 16, 2012 at 5:40 PM, Chandler Carruth <chandlerc at google.com>wrote:

> On Mon, Jul 16, 2012 at 5:27 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> On Mon, Jul 16, 2012 at 5:15 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>>
>>>
>>> On Jul 16, 2012, at 17:06 , Richard Smith <richard-llvm at metafoo.co.uk>
>>> wrote:
>>>
>>> int test6() {
>>>   int x; // expected-note{{initialize the variable 'x' to silence this
>>> warning}}
>>> -  x += 2; // expected-warning{{variable 'x' is uninitialized when used
>>> here}}
>>> -  return x;
>>> +  x += 2;
>>> +  return x; // expected-warning{{variable 'x' is uninitialized when
>>> used here}}
>>> }
>>>
>>>
>>> Sorry for not chiming in pre-commit, but I'm not happy with this. The
>>> first non-compound-assignment use of a variable could be very far from the
>>> variable, and while we do get a note on the decl, it still seems like a QoI
>>> problem. I'd rather have compound assignments treated as a use *and* an
>>> initialization.
>>>
>>> (Actually, can we just call it a use? Does that hurt anything?)
>>>
>>
>> I mentioned this on the review thread: it introduced "false" positives
>> into my test corpus (FSVO "false"). Basically, if a variable is only ever
>> used in compound-assignments, but its value is never actually used, we have
>> not proven that there is a "real" problem with the code (FSVO "real").
>>
>> I don't see how the distance between the variable and the use which we
>> report is a problem, since we give diagnostics pointing at both of them. I
>> would prefer to not add this extra checking (which seems to basically be
>> all noise) into -Wuninitialized, but if you feel strongly about this, I'll
>> make the change.
>>
>
> While using your condition for triggering the warning at all, could we
> attach the warning to the 'x += 2', and note the final use? Or at least
> note the 'x += 2'? That would seem to address most of Jordan's QoI concern
> w/o increasing false positives.
>

That doesn't fall naturally out of the algorithm, so I'd be concerned that
the added complexity doesn't justify the benefit.

I'm coming round to Jordy's position. In particular, for "int n; n *= 0;
return n;", warning on the return statement does not seem particularly
reasonable, and warning on the *= is pedantically correct, since it has
undefined behavior. The code samples in which I found the 'false' positives
are fairly questionable.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120716/0a9cc880/attachment.html>