[cfe-commits] [Patch] Warn about self-initialization of reference variables

[Richard Smith]

On Thu, Aug 16, 2012 at 9:21 AM, Hans Wennborg <hans at chromium.org> wrote:

> On Thu, Aug 16, 2012 at 5:09 PM, Jordan Rose <jordan_rose at apple.com>
> wrote:
> > I'm trying to wrap my head around what this means and failing. 'int *a =
> *a'?
>
> That isn't as sneaky as the reference case, because "*a" on the
> right-hand side has type int, so you'd get a warning about trying to
> initialize a pointer with an int.
>
> The self-initialized reference case is also extra bad because we know
> it's going to be invalid forever since it cannot be changed. A badly
> initialized pointer can at least be made valid later.
>
> > I wouldn't worry about the 'a(b), b(a)' case. Or rather, the issue is
> that 'b' is uninitialized in 'a(b)', and it doesn't really matter if it's
> given a valid initialization later.
>
> Yeah, you're right. The reason I've got both a(b) and b(a) in there is
> because otherwise there'd be an error about one of them not getting
> initialized in the constructor.
>
> > I'm not so familiar with this part of Sema, but the diff looks
> reasonable.
>
> Thanks! Anyone else who'd like to take a look?


LGTM, modulo a couple of small things:

> @@ -6309,13 +6312,13 @@ void Sema::AddInitializerToDecl(Decl *RealDecl,
Expr *Init,
>    }
>
>    // Check for self-references within variable initializers.
> -  // Variables declared within a function/method body are handled
> -  // by a dataflow analysis.
> +  // Variables declared within a function/method body (except for
references)
> +  // are handled by a dataflow analysis.
>    // Record types initialized by initializer list are handled here.
>    // Initialization by constructors are handled in
TryConstructorInitialization.
> -  if (!VDecl->hasLocalStorage() &&
> -      (isa<InitListExpr>(Init) || !VDecl->getType()->isRecordType()))
> -    CheckSelfReference(RealDecl, Init);
> +  if (!VDecl->hasLocalStorage() || VDecl->getType()->isReferenceType())
> +    if (isa<InitListExpr>(Init) || !VDecl->getType()->isRecordType())
> +      CheckSelfReference(RealDecl, Init);

'&&' instead of nested 'if's? It might be clearer to reuse !isTrackedVar()
(from Analysis/UninitializedValues.cpp) here, but I'm happy for the patch
to go in either way.

> +++ b/test/SemaCXX/convert-to-bool.cpp
> @@ -62,6 +62,5 @@ struct C {
>
>  void test_copy_init_conversions(C c) {
>    A &a = c; // expected-error{{no viable conversion from 'C' to 'A'}}
> -  B &b = b; // okay
> +  B &b = b; // okay expected-warning{{variable 'b' is uninitialized when
used within its own initialization}}

Your warning found a bug in the test! I'm pretty sure this intended to say:
B &b = c;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120816/b58246ab/attachment.html>