[PATCH] D16552: Implement the likely resolution of core issue 253.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/DeclCXX.h:405-416
@@ -404,1 +404,14 @@
 
+    enum AllowConstDefInitKind {
+       ACDI_Unknown,
+       ACDI_Yes,
+       ACDI_No,
+    };
+    unsigned AllowConstDefaultInit : 3;
+    AllowConstDefInitKind allowConstDefInitKind() {
+      return static_cast<AllowConstDefInitKind>(AllowConstDefaultInit);
+    }
+    void setAllowConstDefInitKind(AllowConstDefInitKind Allow) {
+      AllowConstDefaultInit = Allow;
+    }
+
----------------
Is it worth caching this? (If so, the bit-field should occupy at most 2 bits...)

================
Comment at: include/clang/AST/DeclCXX.h:1286
@@ -1272,1 +1285,3 @@
 
+  /// \brief Determine whether declaring a const with this type is ok per
+  /// core issue 253.
----------------
Declaring a const what?

================
Comment at: lib/AST/DeclCXX.cpp:396-425
@@ -394,1 +395,32 @@
 
+bool CXXRecordDecl::allowConstDefaultInitSlow() const {
+  assert(getDefinition() && "only call this on completed records");
+  if (hasUserProvidedDefaultConstructor()) {
+    data().setAllowConstDefInitKind(DefinitionData::ACDI_Yes);
+    return true;
+  }
+  for (const auto *F : fields()) {
+    if (F->hasInClassInitializer() || F->isUnnamedBitfield())
+      continue;
+    if (CXXRecordDecl *FieldType = F->getType()->getAsCXXRecordDecl()) {
+      if (!FieldType->allowConstDefaultInit()) {
+        data().setAllowConstDefInitKind(DefinitionData::ACDI_No);
+        return false;
+      }
+    } else {
+      data().setAllowConstDefInitKind(DefinitionData::ACDI_No);
+      return false;
+    }
+  }
+  for (const auto& BI : bases()) {
+    const RecordType *RT = BI.getType()->getAs<RecordType>();
+    CXXRecordDecl *Base = cast<CXXRecordDecl>(RT->getDecl());
+    if (!Base->allowConstDefaultInit()) {
+      data().setAllowConstDefInitKind(DefinitionData::ACDI_No);
+      return false;
+    }
+  }
+  data().setAllowConstDefInitKind(DefinitionData::ACDI_Yes);
+  return true;
+}
+
----------------
For all the other fields of this kind, we compute the flag as bases and members are added to the class. It seems like we can do the same here, and all else being equal, it would be better to not deviate from the existing pattern without good reason.

The most natural way to do this would probably be to store a single flag for `HasUninitializedFields`, and then compute whether default initialization would leave a field uninitialized as `!hasUserProvidedDefaultConstructor() && HasUninitializedFields`.

================
Comment at: lib/AST/DeclCXX.cpp:403-413
@@ +402,13 @@
+  for (const auto *F : fields()) {
+    if (F->hasInClassInitializer() || F->isUnnamedBitfield())
+      continue;
+    if (CXXRecordDecl *FieldType = F->getType()->getAsCXXRecordDecl()) {
+      if (!FieldType->allowConstDefaultInit()) {
+        data().setAllowConstDefInitKind(DefinitionData::ACDI_No);
+        return false;
+      }
+    } else {
+      data().setAllowConstDefInitKind(DefinitionData::ACDI_No);
+      return false;
+    }
+  }
----------------
I think you should skip `mutable` members here, as the corresponding subobject of a `const` object is not itself `const`.


http://reviews.llvm.org/D16552