[cfe-commits] [Patch] Mark pass-by-reference as uninitialized variable use on initialization

[Richard Trieu]

On Tue, Aug 14, 2012 at 4:15 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Tue, Aug 14, 2012 at 4:01 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> On Tue, Aug 14, 2012 at 3:31 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> On Mon, Aug 6, 2012 at 6:47 PM, Richard Trieu <rtrieu at google.com> wrote:
>>>
>>>> On Mon, Aug 6, 2012 at 4:09 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>>>
>>>>> On Mon, Aug 6, 2012 at 3:29 PM, Richard Trieu <rtrieu at google.com>wrote:
>>>>>
>>>>>> Modify the self-reference visitor to mark pass-by-reference to
>>>>>> function calls as uninitialized use.  This will warn on code such as:
>>>>>>
>>>>>> const string Foo = "prefix" + Foo + "suffix";
>>>>>>
>>>>>
>>>>> Can we put this under a more specific -W flag? -Wuninitialized is
>>>>> intended to be essentially free of false positives.
>>>>>
>>>> Would this be more suited for a different uninitialized group or a new
>>>> flag altogether?
>>>>
>>>
>>> I think a different warning flag would probably be best; neither
>>> -Wuninitialized nor -Wconditional-uninitialized really fits well.
>>>
>>>
>>>> I'm a little concerned that this uninitialized-variables warning is
>>>>> deviating from the behavior of the CFG-based warning, which explicitly
>>>>> excludes variables passed by reference to functions from its list of uses,
>>>>> so if this is effective at finding bugs, perhaps we should add this check
>>>>> there too.
>>>>>
>>>> At least for classes, this seems to be a reasonable check.  I'm not
>>>> sure if it would be useful for other cases.
>>>>
>>>>>
>>>>> --- lib/Sema/SemaDecl.cpp (revision 161345)
>>>>> +++ lib/Sema/SemaDecl.cpp (working copy)
>>>>> @@ -6223,7 +6223,7 @@
>>>>>
>>>>>      void VisitImplicitCastExpr(ImplicitCastExpr *E) {
>>>>>        if ((!isRecordType && E->getCastKind() == CK_LValueToRValue) ||
>>>>> -          (isRecordType && E->getCastKind() == CK_NoOp))
>>>>> +          (E->getCastKind() == CK_NoOp))
>>>>>
>>>>> Drop these parens... But I'm not really clear on what this is checking
>>>>> for. Presumably qualification conversions, but why are those a good place
>>>>> to look for self-initialization?
>>>>>
>>>> T -> const T conversion when passed by reference.
>>>
>>>
>>> OK, but does that mean we don't catch:
>>>
>>> const string s = "Foo " + s + " bar";
>>>
>>> ? It would make more sense to me to check const
>>> glvalue arguments to function call (including overloaded operator calls
>>> etc).
>>>
>> const string s = "Foo " + s + " bar"; is the motivating case for this
>> change, which is uncaught by current -Wuninitialized.  It's caught by
>> VisitCallExpr() where it any argument that is just a DeclRefExpr is
>> checked.
>
>
> OK, then it'd make more sense to me for that check to look through casts
> (at least lvalue bitcasts, no-op conversions and lvalue-to-rvalue
> conversions, but possibly others) than to start a search from no-op
> conversions.
>

Changed the checks on casts to include the above, and do so for all casts,
not just implicit casts.
Added test cases for lvalue bit cast.

What would be the proper flag for warning?  -Wreference-uninitialized?
 -Wpossibly-uninitalized?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120827/f47a1a5e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: uninitialized-reference2.patch
Type: application/octet-stream
Size: 11179 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120827/f47a1a5e/attachment.obj>