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

[Nico Weber via cfe-commits]

thakis added a comment.

thanks!


================
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;
+    }
+
----------------
rsmith wrote:
> Is it worth caching this? (If so, the bit-field should occupy at most 2 bits...)
The motivation is that a chain of

struct A0 {};
struct A1 {  const A0 a; }
struct A2 {  const A1 a; }
struct A3 {  const A2 a; }

needs O(n^2) without caching. Changed the bitfield to 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.
----------------
rsmith wrote:
> Declaring a const what?
Done (I think?)

================
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;
+}
+
----------------
rsmith wrote:
> 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`.
My reasoning here was that for the vast majority of types, we'll never have a const variable of that type, so computing this for all types seemed like a waste.


http://reviews.llvm.org/D16552