During cross field uninitialized checking, when processing an assignment,

don't mark the field as initialized until the next initializer instead of
instantly.  Since this checker is AST based, statements are processed in tree
order instead of following code flow.  This can result in different warnings
from just reordering the code.  Also changed to use one checker per constructor
instead of creating a new checker per field.

class T {
  int x, y;

  // Already warns
  T(bool b) : x(!b ? (1 + y) : (y = 5)) {}

  // New warning added here, previously (1 + y) comes after (y = 5) in the AST
  // preventing the warning.
  T(bool b) : x(b ? (y = 5) : (1 + y)) {}

};

llvm-svn: 216641

clang/lib/Sema/SemaDeclCXX.cpp

@@ -2210,15 +2210,16 @@ namespace {
     // List of Decls to generate a warning on.  Also remove Decls that become
     // initialized.
     llvm::SmallPtrSetImpl<ValueDecl*> &Decls;
+    // Vector of decls to be removed from the Decl set prior to visiting the
+    // nodes.  These Decls may have been initialized in the prior initializer.
+    llvm::SmallVector<ValueDecl*, 4> DeclsToRemove;
     // If non-null, add a note to the warning pointing back to the constructor.
     const CXXConstructorDecl *Constructor;
   public:
     typedef EvaluatedExprVisitor<UninitializedFieldVisitor> Inherited;
     UninitializedFieldVisitor(Sema &S,
-                              llvm::SmallPtrSetImpl<ValueDecl*> &Decls,
-                              const CXXConstructorDecl *Constructor)
-      : Inherited(S.Context), S(S), Decls(Decls),
-        Constructor(Constructor) { }
+                              llvm::SmallPtrSetImpl<ValueDecl*> &Decls)
+      : Inherited(S.Context), S(S), Decls(Decls) { }
 
     void HandleMemberExpr(MemberExpr *ME, bool CheckReferenceOnly) {
       if (isa<EnumConstantDecl>(ME->getMemberDecl()))
@@ -2307,6 +2308,20 @@ namespace {
       }
     }
 
+    void CheckInitializer(Expr *E, const CXXConstructorDecl *FieldConstructor,
+                          FieldDecl *Field) {
+      // Remove Decls that may have been initialized in the previous
+      // initializer.
+      for (ValueDecl* VD : DeclsToRemove)
+        Decls.erase(VD);
+
+      DeclsToRemove.clear();
+      Constructor = FieldConstructor;
+      Visit(E);
+      if (Field)
+        Decls.erase(Field);
+    }
+
     void VisitMemberExpr(MemberExpr *ME) {
       // All uses of unbounded reference fields will warn.
       HandleMemberExpr(ME, true /*CheckReferenceOnly*/);
@@ -2365,30 +2380,11 @@ namespace {
         if (MemberExpr *ME = dyn_cast<MemberExpr>(E->getLHS()))
           if (FieldDecl *FD = dyn_cast<FieldDecl>(ME->getMemberDecl()))
             if (!FD->getType()->isReferenceType())
-              Decls.erase(FD);
+              DeclsToRemove.push_back(FD);
 
       Inherited::VisitBinaryOperator(E);
     }
   };
-  static void CheckInitExprContainsUninitializedFields(
-      Sema &S, Expr *E, llvm::SmallPtrSetImpl<ValueDecl*> &Decls,
-      const CXXConstructorDecl *Constructor) {
-    if (Decls.size() == 0)
-      return;
-
-    if (!E)
-      return;
-
-    if (CXXDefaultInitExpr *Default = dyn_cast<CXXDefaultInitExpr>(E)) {
-      E = Default->getExpr();
-      if (!E)
-        return;
-      // In class initializers will point to the constructor.
-      UninitializedFieldVisitor(S, Decls, Constructor).Visit(E);
-    } else {
-      UninitializedFieldVisitor(S, Decls, nullptr).Visit(E);
-    }
-  }
 
   // Diagnose value-uses of fields to initialize themselves, e.g.
   //   foo(foo)
@@ -2421,14 +2417,32 @@ namespace {
       }
     }
 
+    if (UninitializedFields.empty())
+      return;
+
+    UninitializedFieldVisitor UninitializedChecker(SemaRef,
+                                                   UninitializedFields);
+
     for (const auto *FieldInit : Constructor->inits()) {
+      if (UninitializedFields.empty())
+        break;
+
       Expr *InitExpr = FieldInit->getInit();
+      if (!InitExpr)
+        continue;
 
-      CheckInitExprContainsUninitializedFields(
-          SemaRef, InitExpr, UninitializedFields, Constructor);
-
-      if (FieldDecl *Field = FieldInit->getAnyMember())
-        UninitializedFields.erase(Field);
+      if (CXXDefaultInitExpr *Default =
+              dyn_cast<CXXDefaultInitExpr>(InitExpr)) {
+        InitExpr = Default->getExpr();
+        if (!InitExpr)
+          continue;
+        // In class initializers will point to the constructor.
+        UninitializedChecker.CheckInitializer(InitExpr, Constructor,
+                                              FieldInit->getAnyMember());
+      } else {
+        UninitializedChecker.CheckInitializer(InitExpr, nullptr,
+                                              FieldInit->getAnyMember());
+      }
     }
   }
 } // namespace

clang/test/SemaCXX/uninitialized.cpp

@@ -823,6 +823,20 @@ namespace cross_field_warnings {
     int d = a + b + c;
     R() : a(c = 5), b(c), c(a) {}
   };
+
+  // FIXME: Use the CFG-based analysis to give a sometimes uninitialized
+  // warning on y.
+  struct T {
+    int x;
+    int y;
+    T(bool b)
+        : x(b ? (y = 5) : (1 + y)),  // expected-warning{{field 'y' is uninitialized when used here}}
+          y(y + 1) {}
+    T(int b)
+        : x(!b ? (1 + y) : (y = 5)),  // expected-warning{{field 'y' is uninitialized when used here}}
+          y(y + 1) {}
+  };
+
 }
 
 namespace base_class {