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

[Hans Wennborg]

On Fri, Aug 17, 2012 at 9:20 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> >> -  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.
>>
>> Switched to && instead of nested ifs for now.
>>
>> !isTrackedVar() doesn't seem to be a one-to-one match here. For
>> example, in my if statement I'm checking that it's not a record type,
>> whereas !isTrackedVar() would check that it's not a scalar or vector
>> type. Happy to work with this post-commit if you think it would be
>> better, though.
>
> Yeah, isTrackedVar() only corresponds (approximately) to the LHS of your &&.
> The RHS is dealing with this check also being performed in SemaInit for
> non-InitListExpr record initializations.

I tried something like this:

+  if (!isUninitVariablesAnalysisTracked(VDecl, VDecl->getDeclContext()) &&
+      (!VDecl->getType()->isRecordType() || isa<InitListExpr>(Init)))
+    CheckSelfReference(RealDecl, Init);

But it turns out that !isTrackedVar() is a much more allowing
conditional than !VDecl->hasLocalStorage() ||
VDecl->getType()->isReferenceType().

If I use the !isTrackedVar() variant, a bunch of other variables slip
through and I hit some asserts in the test suite.

I'll just leave it as it is if that's ok.

Thanks,
Hans