Simplify and refactor the uninitialized field warning.

Change the uninitialized field warnings so that field initializers are checked
inside the constructor.  Previously, in class initializers were checked
separately.  Running one set of checks also simplifies the logic for preventing
duplicate warnings.  Added new checks to warn when an uninitialized field is
used in base class initialization.  Also fixed misspelling of uninitialized
and moved all code for this warning together.

llvm-svn: 193386

3 files changed, 155 insertions, 194 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 7abb74cd0b7..21aad683eac 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1371,7 +1371,8 @@ def warn_reference_field_is_uninit : Warning<
   "reference %0 is not yet bound to a value when used here">,
   InGroup<Uninitialized>;
 def note_uninit_in_this_constructor : Note<
-  "during field initialization in this constructor">;
+  "during field initialization in %select{this|the implicit default}0 "
+  "constructor">;
 def warn_static_self_reference_in_init : Warning<
   "static variable %0 is suspiciously used within its own initialization">,
   InGroup<UninitializedStaticSelfInit>;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 221c53f03f6..3b1f67c07fe 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -2110,40 +2110,20 @@ namespace {
   class UninitializedFieldVisitor
       : public EvaluatedExprVisitor<UninitializedFieldVisitor> {
     Sema &S;
-    // If VD is null, this visitor will only update the Decls set.
-    ValueDecl *VD;
-    bool isReferenceType;
-    // List of Decls to generate a warning on.
+    // List of Decls to generate a warning on.  Also remove Decls that become
+    // initialized.
     llvm::SmallPtrSet<ValueDecl*, 4> &Decls;
-    bool WarnOnSelfReference;
     // 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, ValueDecl *VD,
+    UninitializedFieldVisitor(Sema &S,
                               llvm::SmallPtrSet<ValueDecl*, 4> &Decls,
-                              bool WarnOnSelfReference,
                               const CXXConstructorDecl *Constructor)
-      : Inherited(S.Context), S(S), VD(VD), isReferenceType(false), Decls(Decls),
-        WarnOnSelfReference(WarnOnSelfReference), Constructor(Constructor) {
-      // When VD is null, this visitor is used to detect initialization of other
-      // fields.
-      if (VD) {
-        if (IndirectFieldDecl *IFD = dyn_cast<IndirectFieldDecl>(VD))
-          this->VD = IFD->getAnonField();
-        else
-          this->VD = VD;
-        isReferenceType = this->VD->getType()->isReferenceType();
-      }
-    }
+      : Inherited(S.Context), S(S), Decls(Decls),
+        Constructor(Constructor) { }
 
     void HandleMemberExpr(MemberExpr *ME, bool CheckReferenceOnly) {
-      if (!VD)
-        return;
-
-      if (CheckReferenceOnly && !isReferenceType)
-        return;
-
       if (isa<EnumConstantDecl>(ME->getMemberDecl()))
         return;
 
@@ -2170,36 +2150,27 @@ namespace {
 
       ValueDecl* FoundVD = FieldME->getMemberDecl();
 
-      if (VD == FoundVD) {
-        if (!WarnOnSelfReference)
-          return;
-
-        unsigned diag = isReferenceType
-            ? diag::warn_reference_field_is_uninit
-            : diag::warn_field_is_uninit;
-        S.Diag(FieldME->getExprLoc(), diag) << VD;
-        if (Constructor)
-          S.Diag(Constructor->getLocation(),
-                 diag::note_uninit_in_this_constructor);
+      if (!Decls.count(FoundVD))
         return;
-      }
 
-      if (CheckReferenceOnly)
+      const bool IsReference = FoundVD->getType()->isReferenceType();
+
+      // Prevent double warnings on use of unbounded references.
+      if (IsReference != CheckReferenceOnly)
         return;
 
-      if (Decls.count(FoundVD)) {
-        S.Diag(FieldME->getExprLoc(), diag::warn_field_is_uninit) << FoundVD;
-        if (Constructor)
-          S.Diag(Constructor->getLocation(),
-                 diag::note_uninit_in_this_constructor);
+      unsigned diag = IsReference
+          ? diag::warn_reference_field_is_uninit
+          : diag::warn_field_is_uninit;
+      S.Diag(FieldME->getExprLoc(), diag) << FoundVD;
+      if (Constructor)
+        S.Diag(Constructor->getLocation(),
+               diag::note_uninit_in_this_constructor)
+          << (Constructor->isDefaultConstructor() && Constructor->isImplicit());
 
-      }
     }
 
     void HandleValue(Expr *E) {
-      if (!VD)
-        return;
-
       E = E->IgnoreParens();
 
       if (MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
@@ -2236,6 +2207,7 @@ namespace {
     }
 
     void VisitMemberExpr(MemberExpr *ME) {
+      // All uses of unbounded reference fields will warn.
       HandleMemberExpr(ME, true /*CheckReferenceOnly*/);
 
       Inherited::VisitMemberExpr(ME);
@@ -2272,20 +2244,78 @@ namespace {
       if (E->getOpcode() == BO_Assign)
         if (MemberExpr *ME = dyn_cast<MemberExpr>(E->getLHS()))
           if (FieldDecl *FD = dyn_cast<FieldDecl>(ME->getMemberDecl()))
-            Decls.erase(FD);
+            if (!FD->getType()->isReferenceType())
+              Decls.erase(FD);
 
       Inherited::VisitBinaryOperator(E);
     }
   };
   static void CheckInitExprContainsUninitializedFields(
-      Sema &S, Expr *E, ValueDecl *VD, llvm::SmallPtrSet<ValueDecl*, 4> &Decls,
-      bool WarnOnSelfReference, const CXXConstructorDecl *Constructor = 0) {
-    if (Decls.size() == 0 && !WarnOnSelfReference)
+      Sema &S, Expr *E, llvm::SmallPtrSet<ValueDecl*, 4> &Decls,
+      const CXXConstructorDecl *Constructor) {
+    if (Decls.size() == 0)
       return;
 
-    if (E)
-      UninitializedFieldVisitor(S, VD, Decls, WarnOnSelfReference, Constructor)
-          .Visit(E);
+    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, 0).Visit(E);
+    }
+  }
+
+  // Diagnose value-uses of fields to initialize themselves, e.g.
+  //   foo(foo)
+  // where foo is not also a parameter to the constructor.
+  // Also diagnose across field uninitialized use such as
+  //   x(y), y(x)
+  // TODO: implement -Wuninitialized and fold this into that framework.
+  static void DiagnoseUninitializedFields(
+      Sema &SemaRef, const CXXConstructorDecl *Constructor) {
+
+    if (SemaRef.getDiagnostics().getDiagnosticLevel(diag::warn_field_is_uninit,
+                                                    Constructor->getLocation())
+        == DiagnosticsEngine::Ignored) {
+      return;
+    }
+
+    if (Constructor->isInvalidDecl())
+      return;
+
+    const CXXRecordDecl *RD = Constructor->getParent();
+
+    // Holds fields that are uninitialized.
+    llvm::SmallPtrSet<ValueDecl*, 4> UninitializedFields;
+
+    // At the beginning, all fields are uninitialized.
+    for (DeclContext::decl_iterator I = RD->decls_begin(), E = RD->decls_end();
+         I != E; ++I) {
+      if (FieldDecl *FD = dyn_cast<FieldDecl>(*I)) {
+        UninitializedFields.insert(FD);
+      } else if (IndirectFieldDecl *IFD = dyn_cast<IndirectFieldDecl>(*I)) {
+        UninitializedFields.insert(IFD->getAnonField());
+      }
+    }
+
+    for (CXXConstructorDecl::init_const_iterator FieldInit =
+             Constructor->init_begin(),
+             FieldInitEnd = Constructor->init_end();
+         FieldInit != FieldInitEnd; ++FieldInit) {
+
+      Expr *InitExpr = (*FieldInit)->getInit();
+
+      CheckInitExprContainsUninitializedFields(
+          SemaRef, InitExpr, UninitializedFields, Constructor);
+
+      if (FieldDecl *Field = (*FieldInit)->getAnyMember())
+        UninitializedFields.erase(Field);
+    }
   }
 } // namespace
 
@@ -3779,90 +3809,6 @@ bool CheckRedundantUnionInit(Sema &S,
 }
 }
 
-// Diagnose value-uses of fields to initialize themselves, e.g.
-//   foo(foo)
-// where foo is not also a parameter to the constructor.
-// Also diagnose across field uninitialized use such as
-//   x(y), y(x)
-// TODO: implement -Wuninitialized and fold this into that framework.
-static void DiagnoseUnitializedFields(
-    Sema &SemaRef, const CXXConstructorDecl *Constructor) {
-
-  if (SemaRef.getDiagnostics().getDiagnosticLevel(diag::warn_field_is_uninit,
-                                                  Constructor->getLocation())
-      == DiagnosticsEngine::Ignored) {
-    return;
-  }
-
-  const CXXRecordDecl *RD = Constructor->getParent();
-
-  // Holds fields that are uninitialized.
-  llvm::SmallPtrSet<ValueDecl*, 4> UninitializedFields;
-
-  for (DeclContext::decl_iterator I = RD->decls_begin(), E = RD->decls_end();
-       I != E; ++I) {
-    if (FieldDecl *FD = dyn_cast<FieldDecl>(*I)) {
-      UninitializedFields.insert(FD);
-    } else if (IndirectFieldDecl *IFD = dyn_cast<IndirectFieldDecl>(*I)) {
-      UninitializedFields.insert(IFD->getAnonField());
-    }
-  }
-
-  // Fields already checked when processing the in class initializers.
-  llvm::SmallPtrSet<ValueDecl*, 4>
-      InClassUninitializedFields = UninitializedFields;
-
-  for (CXXConstructorDecl::init_const_iterator FieldInit =
-           Constructor->init_begin(),
-           FieldInitEnd = Constructor->init_end();
-       FieldInit != FieldInitEnd; ++FieldInit) {
-
-    FieldDecl *Field = (*FieldInit)->getAnyMember();
-    Expr *InitExpr = (*FieldInit)->getInit();
-
-    if (!Field) {
-      CheckInitExprContainsUninitializedFields(
-          SemaRef, InitExpr, 0, UninitializedFields,
-          false/*WarnOnSelfReference*/);
-      continue;
-    }
-
-    if (CXXDefaultInitExpr *Default = dyn_cast<CXXDefaultInitExpr>(InitExpr)) {
-      // This field is initialized with an in-class initailzer.  Remove the
-      // fields already checked to prevent duplicate warnings.
-      llvm::SmallPtrSet<ValueDecl*, 4> DiffSet = UninitializedFields;
-      for (llvm::SmallPtrSet<ValueDecl*, 4>::iterator
-               I = InClassUninitializedFields.begin(),
-               E = InClassUninitializedFields.end();
-           I != E; ++I) {
-        DiffSet.erase(*I);
-      }
-      CheckInitExprContainsUninitializedFields(
-            SemaRef, Default->getExpr(), Field, DiffSet,
-            DiffSet.count(Field), Constructor);
-
-      // Update the unitialized field sets.
-      CheckInitExprContainsUninitializedFields(
-            SemaRef, Default->getExpr(), 0, UninitializedFields,
-            false);
-      CheckInitExprContainsUninitializedFields(
-            SemaRef, Default->getExpr(), 0, InClassUninitializedFields,
-            false);
-    } else {
-      CheckInitExprContainsUninitializedFields(
-          SemaRef, InitExpr, Field, UninitializedFields,
-          UninitializedFields.count(Field));
-      if (Expr* InClassInit = Field->getInClassInitializer()) {
-        CheckInitExprContainsUninitializedFields(
-            SemaRef, InClassInit, 0, InClassUninitializedFields,
-            false);
-      }
-    }
-    UninitializedFields.erase(Field);
-    InClassUninitializedFields.erase(Field);
-  }
-}
-
 /// ActOnMemInitializers - Handle the member initializers for a constructor.
 void Sema::ActOnMemInitializers(Decl *ConstructorDecl,
                                 SourceLocation ColonLoc,
@@ -3928,7 +3874,7 @@ void Sema::ActOnMemInitializers(Decl *ConstructorDecl,
 
   SetCtorInitializers(Constructor, AnyErrors, MemInits);
 
-  DiagnoseUnitializedFields(*this, Constructor);
+  DiagnoseUninitializedFields(*this, Constructor);
 }
 
 void
@@ -4056,8 +4002,10 @@ void Sema::ActOnDefaultCtorInitializers(Decl *CDtorDecl) {
     return;
 
   if (CXXConstructorDecl *Constructor
-      = dyn_cast<CXXConstructorDecl>(CDtorDecl))
+      = dyn_cast<CXXConstructorDecl>(CDtorDecl)) {
     SetCtorInitializers(Constructor, /*AnyErrors=*/false);
+    DiagnoseUninitializedFields(*this, Constructor);
+  }
 }
 
 bool Sema::RequireNonAbstractType(SourceLocation Loc, QualType T,
@@ -8215,61 +8163,13 @@ void Sema::DefineImplicitDefaultConstructor(SourceLocation CurrentLocation,
   if (ASTMutationListener *L = getASTMutationListener()) {
     L->CompletedImplicitDefinition(Constructor);
   }
+
+  DiagnoseUninitializedFields(*this, Constructor);
 }
 
 void Sema::ActOnFinishDelayedMemberInitializers(Decl *D) {
   // Perform any delayed checks on exception specifications.
   CheckDelayedMemberExceptionSpecs();
-
-  // Once all the member initializers are processed, perform checks to see if
-  // any unintialized use is happeneing.
-  if (getDiagnostics().getDiagnosticLevel(diag::warn_field_is_uninit,
-                                          D->getLocation())
-      == DiagnosticsEngine::Ignored)
-    return;
-
-  CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D);
-  if (!RD) return;
-
-  // Holds fields that are uninitialized.
-  llvm::SmallPtrSet<ValueDecl*, 4> UninitializedFields;
-
-  // In the beginning, every field is uninitialized.
-  for (DeclContext::decl_iterator I = RD->decls_begin(), E = RD->decls_end();
-       I != E; ++I) {
-    if (FieldDecl *FD = dyn_cast<FieldDecl>(*I)) {
-      UninitializedFields.insert(FD);
-    } else if (IndirectFieldDecl *IFD = dyn_cast<IndirectFieldDecl>(*I)) {
-      UninitializedFields.insert(IFD->getAnonField());
-    }
-  }
-
-  for (DeclContext::decl_iterator I = RD->decls_begin(), E = RD->decls_end();
-       I != E; ++I) {
-    FieldDecl *FD = dyn_cast<FieldDecl>(*I);
-    if (!FD)
-      if (IndirectFieldDecl *IFD = dyn_cast<IndirectFieldDecl>(*I))
-        FD = IFD->getAnonField();
-
-    if (!FD)
-      continue;
-
-    Expr *InitExpr = FD->getInClassInitializer();
-    if (!InitExpr) {
-      // Uninitialized reference types will give an error.
-      // Record types with an initializer are default initialized.
-      QualType FieldType = FD->getType();
-      if (FieldType->isReferenceType() || FieldType->isRecordType())
-        UninitializedFields.erase(FD);
-      continue;
-    }
-
-    CheckInitExprContainsUninitializedFields(
-        *this, InitExpr, FD, UninitializedFields,
-        UninitializedFields.count(FD)/*WarnOnSelfReference*/);
-
-    UninitializedFields.erase(FD);
-  }
 }
 
 namespace {
diff --git a/clang/test/SemaCXX/uninitialized.cpp b/clang/test/SemaCXX/uninitialized.cpp
index 2fb0482cacb..4991ebee8b3 100644
--- a/clang/test/SemaCXX/uninitialized.cpp
+++ b/clang/test/SemaCXX/uninitialized.cpp
@@ -487,7 +487,8 @@ namespace references {
   }
 
   struct T {
-    T() : a(b), b(a) {} // FIXME: Warn here.
+    T() // expected-note{{during field initialization in this constructor}}
+     : a(b), b(a) {} // expected-warning{{reference 'b' is not yet bound to a value when used here}}
     int &a, &b;
     int &c = c; // expected-warning{{reference 'c' is not yet bound to a value when used here}}
   };
@@ -552,7 +553,9 @@ namespace record_fields {
     B(char (*)[7]) : a(const_ref(a)) {}
     B(char (*)[8]) : a(pointer(&a)) {}
     B(char (*)[9]) : a(normal(a)) {}  // expected-warning {{uninitialized}}
-
+  };
+  struct C {
+    C() {} // expected-note4{{in this constructor}}
     A a1 = a1;  // expected-warning {{uninitialized}}
     A a2 = a2.get();  // expected-warning {{uninitialized}}
     A a3 = a3.num();
@@ -563,6 +566,29 @@ namespace record_fields {
     A a8 = pointer(&a8);
     A a9 = normal(a9);  // expected-warning {{uninitialized}}
   };
+  struct D {  // expected-note4{{in the implicit default constructor}}
+    A a1 = a1;  // expected-warning {{uninitialized}}
+    A a2 = a2.get();  // expected-warning {{uninitialized}}
+    A a3 = a3.num();
+    A a4 = a4.copy(a4);  // expected-warning {{uninitialized}}
+    A a5 = a5.something(a5);
+    A a6 = ref(a6);
+    A a7 = const_ref(a7);
+    A a8 = pointer(&a8);
+    A a9 = normal(a9);  // expected-warning {{uninitialized}}
+  };
+  D d;
+  struct E {
+    A a1 = a1;
+    A a2 = a2.get();
+    A a3 = a3.num();
+    A a4 = a4.copy(a4);
+    A a5 = a5.something(a5);
+    A a6 = ref(a6);
+    A a7 = const_ref(a7);
+    A a8 = pointer(&a8);
+    A a9 = normal(a9);
+  };
 }
 
 namespace cross_field_warnings {
@@ -576,13 +602,14 @@ namespace cross_field_warnings {
   struct B {
     int a = b;  // expected-warning{{field 'b' is uninitialized when used here}}
     int b;
+    B() {} // expected-note{{during field initialization in this constructor}}
   };
 
   struct C {
     int a;
     int b = a;  // expected-warning{{field 'a' is uninitialized when used here}}
     C(char (*)[1]) : a(5) {}
-    C(char (*)[2]) {}
+    C(char (*)[2]) {} // expected-note{{during field initialization in this constructor}}
   };
 
   struct D {
@@ -666,5 +693,38 @@ namespace cross_field_warnings {
     P() : x(o.get()) { }
   };
 
+  struct Q {
+    int a;
+    int b;
+    int &c;
+    Q() :
+      a(c = 5),  // expected-warning{{reference 'c' is not yet bound to a value when used here}}
+      b(c),  // expected-warning{{reference 'c' is not yet bound to a value when used here}}
+      c(a) {}
+  };
+
+  struct R {
+    int a;
+    int b;
+    int c;
+    int d = a + b + c;
+    R() : a(c = 5), b(c), c(a) {}
+  };
 }
 
+namespace base_class {
+  struct A {
+    A (int) {}
+  };
+
+  struct B : public A {
+    int x;
+    B() : A(x) {}   // expected-warning{{field 'x' is uninitialized when used here}}
+  };
+
+  struct C : public A {
+    int x;
+    int y;
+    C() : A(y = 4), x(y) {}
+  };
+}