[cfe-commits] Patch for PR5273

[Brandon Pearcy]

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;
      }


> Relatedly, you should add more negative cases to your test cases: cases
> where we really shouldn't generate the warning because we can't know what's
> happening. For example:
>

Done. Added a few more negative cases.

This isn't specific to copy constructors; please change the comment to not
> lead with that.
>

Done.

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). e.g.

class A {
  int B;
  A() :
    B(Assign(B, 10)) { }   // which is almost equivalent to B(B = 10; B)
};

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.

I'll send out a new patch once I hear back from you.

Thanks for all the comments!
-- 
Brandon
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20091029/2a6e5f22/attachment.html>