[cfe-commits] Patch for PR5273

[John McCall]

static bool IsCXXThisExpr(const Stmt* S) {
+  const CXXThisExpr* ThisExpr = dyn_cast<CXXThisExpr>(S);
+  if (ThisExpr) {
+    return true;
+  }
+  bool found = false;
+  for (Stmt::const_child_iterator it = S->child_begin();
+       it != S->child_end() && found == false;
+       ++it) {
+    found = IsCXXThisExpr(*it);
+  }
+  return found;
+}


This logic is still way too aggressive. The warning should only trigger 
if we're pretty sure the result of the base expression will be the 
'this' object; we cannot assume that of arbitrary expressions involving 
'this'. The correct strategy is to start with the minimal possible case 
— here, isa<CXXThisExpr>(S). If there are cases that doesn't cover that 
you're certain you want to warn about, then add those in specifically — 
for example, casts of this (or casts of casts of this, etc.) — and just 
keep whitelisting expressions until you're happy with the warnings 
you've got.

I would write this function as:

static bool IsCXXThisExpr(const Expr* E) {
while (const CastExpr *CE = dyn_cast<CastExpr>(E))
E = CE->getSubExpr();
return isa<CXXThisExpr>(E);
}

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:

class A {
int B;
A() : B((this-1)->B + 1) {} // should not warn
};


+      // Exception #1:  We may be in a copy-constructor. To eliminate this
+      // possibility, check if this field is rooted in "this". If not, this
+      // field probably belongs to some other record.
+      // e.g. Foo(const Foo& rhs) : A(rhs.A) {}
+      if (!IsCXXThisExpr(S)) {
+        // In a copy-constructor; not a warning.
+        return false;
+      }


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

+      // Exception #2:  A reference operator is valid on an uninitialized field.
+      // e.g. Foo() : A(SomeWrapperOrDecorator(&A)) { }
+      if (PrevS) {
+        const UnaryOperator* UE = dyn_cast<UnaryOperator>(PrevS);
+        if (UE && UE->getOpcode() == UnaryOperator::AddrOf) {
+          // Field was preceded by '&' unary operator, which is perfectly valid.
+          return false;
+        }
+      }


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.

+  // A sanity check: ensure no args contain the member itself, as it is not yet
+  // initialized. e.g. foo(foo) or foo(DoXyz(foo)).
+  for (unsigned i = 0; i < NumArgs; ++i) {
+    SourceLocation L;
+    if (InitExprContainsUninitializedFields(Args[i], NULL, Member, &L)) {
+      Diag(L, diag::warn_field_is_uninit);
+    }
+  }


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.

Otherwise looks pretty good! Thanks for taking this on.

John.