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

[Richard Smith]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120814/5ed28b55/attachment.html>