[cfe-commits] Patch for PR5273

[John McCall]

Brandon Pearcy wrote:
> Hello again John!
>
> As discussed, I have dropped the IsCXXThisExpression in favor of a 
> much simpler couple lines of code:
>
>       const Expr* base = ME->getBase();
>       if (base != NULL && !isa<CXXThisExpr>(base->IgnoreParenCasts())) {
>         // Even though the field matches, it does not belong to this 
> record.
>         return false;
>       }

Excellent.

>     This catches the case where we're taking the address of the
>     argument, but not the case where we're passing it by reference
>     argument to some function.
>
>
> I am sure I am overlooking something, but I thought I'd push back for 
> now. Do we care if it is passed by reference to a function? The only 
> valid operations within that function (that I can imagine) would be to 
> take the address of the uninitialized field, or assign a value to it. 
> The former can be achieved by passing the address into the function, 
> and the latter is just strange considering it is being called from an 
> initialization list).

I agree with Sebastian; we don't want to be policing code style.  If we 
don't understand the code, we have to assume that it's valid.

Like I said in the bug, I think there are common cases we can 
green-light a warning for:  for example, if we're passing an 
uninitialized field by reference to a copy constructor, then sure, let's 
warn about it.  But for arbitrary function calls?  No.

>     This can still warn twice for a single initializer if the
>     initializer has multiple arguments, e.g.
>
>     A() : B(B, B) {}
>
>     Please fix this and add a test to make sure this only warns once.
>
>
> I'm sure there is a good reason to have it only warn once, but I 
> thought it would be better to point out as many potential bugs as 
> possible per pass of the compiler. Do you want the warning to point 
> out both instances of B? Or just the first one? What if we have two 
> different uninitialized fields used in an initializer, e.g.
>
> A() : B(B, C) {} (where C declared after B)
>
> I want this check to eventually report a warning for the use of any 
> and all uninitialized fields, and I am assuming we want to report all 
> of them in a single pass of the compiler.

Thinking twice about it, warning for each use is probably fine.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20091029/547bdf2d/attachment.html>