[clang] Consider aggregate bases when checking if an InitListExpr is constant (PR #80519)

Reid Kleckner via cfe-commits cfe-commits at lists.llvm.org
Thu Feb 8 14:05:54 PST 2024


https://github.com/rnk updated https://github.com/llvm/llvm-project/pull/80519

>From 6ab5ba3f970eaaea542fbed09cae17d3666df6b3 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rnk at google.com>
Date: Sat, 3 Feb 2024 00:18:42 +0000
Subject: [PATCH 1/6] wip

---
 clang/lib/AST/Expr.cpp                  | 12 ++++++++++++
 clang/test/SemaCXX/compound-literal.cpp | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index d665a08deb47e..8852fadf79b9a 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3342,6 +3342,18 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
     if (ILE->getType()->isRecordType()) {
       unsigned ElementNo = 0;
       RecordDecl *RD = ILE->getType()->castAs<RecordType>()->getDecl();
+
+      // Check bases for C++17 aggregate initializers.
+      if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
+        for (unsigned i = 0, e = CXXRD->getNumBases(); i < e; i++) {
+          if (ElementNo < ILE->getNumInits()) {
+            const Expr *Elt = ILE->getInit(ElementNo++);
+            if (!Elt->isConstantInitializer(Ctx, false, Culprit))
+              return false;
+          }
+        }
+      }
+
       for (const auto *Field : RD->fields()) {
         // If this is a union, skip all the fields that aren't being initialized.
         if (RD->isUnion() && ILE->getInitializedFieldInUnion() != Field)
diff --git a/clang/test/SemaCXX/compound-literal.cpp b/clang/test/SemaCXX/compound-literal.cpp
index 5957099de53af..81f8b41ff0313 100644
--- a/clang/test/SemaCXX/compound-literal.cpp
+++ b/clang/test/SemaCXX/compound-literal.cpp
@@ -3,6 +3,7 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify -ast-dump %s > %t-11
 // RUN: FileCheck --input-file=%t-11 %s
 // RUN: FileCheck --input-file=%t-11 %s --check-prefix=CHECK-CXX11
+// RUN: %clang_cc1 -verify -std=c++17 %s
 
 // http://llvm.org/PR7905
 namespace PR7905 {
@@ -108,3 +109,22 @@ int computed_with_lambda = [] {
   return result;
 }();
 #endif
+
+#if __cplusplus >= 201703L
+namespace DynamicFileScopeLiteral {
+// This covers the case where we have a file-scope compound literal with a
+// non-constant initializer in C++. Previously, we had a bug where Clang forgot
+// to consider initializer list elements for bases.
+struct Empty {};
+struct Foo : Empty {
+  int x;
+  int y;
+};
+int f();
+Foo o = (Foo){
+  {},
+  1,
+  f() // expected-error {{initializer element is not a compile-time constant}}
+};
+}
+#endif

>From c630eee1f930c87d2c461de92271c02de220b290 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rnk at google.com>
Date: Mon, 5 Feb 2024 23:47:57 +0000
Subject: [PATCH 2/6] update comments, run test in past language modes, and add
 unit test

---
 clang/lib/AST/Expr.cpp                  |  9 +++-
 clang/test/SemaCXX/compound-literal.cpp | 17 ++++---
 clang/unittests/AST/ASTExprTest.cpp     | 66 ++++++++++++++++++++++---
 3 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 8852fadf79b9a..9190995c1df20 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3328,6 +3328,12 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
            DIUE->getUpdater()->isConstantInitializer(Ctx, false, Culprit);
   }
   case InitListExprClass: {
+    // C++ [temp.dep.expr]p2:
+    //   The elements of an aggregate are:
+    //   — for an array, the array elements in increasing subscript order, or
+    //   — for a class, the direct base classes in declaration order, followed
+    //     by the direct non-static data members (11.4) that are not members of
+    //     an anonymous union, in declaration order.
     const InitListExpr *ILE = cast<InitListExpr>(this);
     assert(ILE->isSemanticForm() && "InitListExpr must be in semantic form");
     if (ILE->getType()->isArrayType()) {
@@ -3343,7 +3349,8 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
       unsigned ElementNo = 0;
       RecordDecl *RD = ILE->getType()->castAs<RecordType>()->getDecl();
 
-      // Check bases for C++17 aggregate initializers.
+      // In C++17, bases were added to the list of members used by aggregate
+      // initialization.
       if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
         for (unsigned i = 0, e = CXXRD->getNumBases(); i < e; i++) {
           if (ElementNo < ILE->getNumInits()) {
diff --git a/clang/test/SemaCXX/compound-literal.cpp b/clang/test/SemaCXX/compound-literal.cpp
index 81f8b41ff0313..a3d3b9faa9fee 100644
--- a/clang/test/SemaCXX/compound-literal.cpp
+++ b/clang/test/SemaCXX/compound-literal.cpp
@@ -110,21 +110,22 @@ int computed_with_lambda = [] {
 }();
 #endif
 
-#if __cplusplus >= 201703L
 namespace DynamicFileScopeLiteral {
 // This covers the case where we have a file-scope compound literal with a
 // non-constant initializer in C++. Previously, we had a bug where Clang forgot
 // to consider initializer list elements for bases.
 struct Empty {};
-struct Foo : Empty {
+struct Foo : Empty { // expected-note 0+ {{candidate constructor}}
   int x;
   int y;
 };
 int f();
-Foo o = (Foo){
-  {},
-  1,
-  f() // expected-error {{initializer element is not a compile-time constant}}
-};
-}
+#if __cplusplus < 201103L
+// expected-error at +6 {{non-aggregate type 'Foo' cannot be initialized with an initializer list}}
+#elif __cplusplus < 201703L
+// expected-error at +4 {{no matching constructor}}
+#else
+// expected-error at +2 {{initializer element is not a compile-time constant}}
 #endif
+Foo o = (Foo){ {}, 1, f() };
+}
diff --git a/clang/unittests/AST/ASTExprTest.cpp b/clang/unittests/AST/ASTExprTest.cpp
index ec75492ccff8e..257ac9c55d1bb 100644
--- a/clang/unittests/AST/ASTExprTest.cpp
+++ b/clang/unittests/AST/ASTExprTest.cpp
@@ -20,17 +20,35 @@
 
 using namespace clang;
 
+using clang::ast_matchers::cxxRecordDecl;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::match;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+
+static IntegerLiteral *createIntLiteral(ASTContext &Ctx, uint32_t Value) {
+  const int numBits = 32;
+  return IntegerLiteral::Create(Ctx, llvm::APInt(numBits, Value),
+                                Ctx.IntTy, {});
+}
+
+const CXXRecordDecl *getCXXRecordDeclNode(ASTUnit *AST, const std::string &Name) {
+  auto Result = match(cxxRecordDecl(hasName(Name)).bind("record"), AST->getASTContext());
+  EXPECT_FALSE(Result.empty());
+  return Result[0].getNodeAs<CXXRecordDecl>("record");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string &Name) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs<VarDecl>("var");
+}
+
 TEST(ASTExpr, IgnoreExprCallbackForwarded) {
   constexpr char Code[] = "";
   auto AST = tooling::buildASTFromCodeWithArgs(Code, /*Args=*/{"-std=c++20"});
   ASTContext &Ctx = AST->getASTContext();
 
-  auto createIntLiteral = [&](uint32_t Value) -> IntegerLiteral * {
-    const int numBits = 32;
-    return IntegerLiteral::Create(Ctx, llvm::APInt(numBits, Value),
-                                  Ctx.UnsignedIntTy, {});
-  };
-
   struct IgnoreParens {
     Expr *operator()(Expr *E) & { return nullptr; }
     Expr *operator()(Expr *E) && {
@@ -42,7 +60,7 @@ TEST(ASTExpr, IgnoreExprCallbackForwarded) {
   };
 
   {
-    auto *IntExpr = createIntLiteral(10);
+    auto *IntExpr = createIntLiteral(Ctx, 10);
     ParenExpr *PE =
         new (Ctx) ParenExpr(SourceLocation{}, SourceLocation{}, IntExpr);
     EXPECT_EQ(IntExpr, IgnoreExprNodes(PE, IgnoreParens{}));
@@ -50,9 +68,41 @@ TEST(ASTExpr, IgnoreExprCallbackForwarded) {
 
   {
     IgnoreParens CB{};
-    auto *IntExpr = createIntLiteral(10);
+    auto *IntExpr = createIntLiteral(Ctx, 10);
     ParenExpr *PE =
         new (Ctx) ParenExpr(SourceLocation{}, SourceLocation{}, IntExpr);
     EXPECT_EQ(nullptr, IgnoreExprNodes(PE, CB));
   }
 }
+
+TEST(ASTExpr, InitListIsConstantInitialized) {
+  auto AST = buildASTFromCode(R"cpp(
+    struct Empty {};
+    struct Foo : Empty { int x, y; };
+    int gv;
+  )cpp");
+  ASTContext &Ctx = AST->getASTContext();
+  const CXXRecordDecl *Empty = getCXXRecordDeclNode(AST.get(), "Empty");
+  const CXXRecordDecl *Foo = getCXXRecordDeclNode(AST.get(), "Foo");
+
+  SourceLocation Loc{};
+  InitListExpr *BaseInit = new (Ctx) InitListExpr(Ctx, Loc, {}, Loc);
+  BaseInit->setType(Ctx.getRecordType(Empty));
+  Expr *Exprs[3] = {
+    BaseInit,
+    createIntLiteral(Ctx, 13),
+    createIntLiteral(Ctx, 42),
+  };
+  InitListExpr *FooInit = new (Ctx) InitListExpr(Ctx, Loc, Exprs, Loc);
+  FooInit->setType(Ctx.getRecordType(Foo));
+  EXPECT_TRUE(FooInit->isConstantInitializer(Ctx, false));
+
+  // Replace the last initializer with something non-constant and make sure
+  // this returns false. Previously we had a bug where we didn't count base
+  // initializers, and only iterated over fields.
+  const VarDecl *GV = getVariableNode(AST.get(), "gv");
+  auto *Ref = new (Ctx) DeclRefExpr(Ctx, const_cast<VarDecl *>(GV), false,
+                                    Ctx.IntTy, VK_LValue, Loc);
+  (void)FooInit->updateInit(Ctx, 2, Ref);
+  EXPECT_FALSE(FooInit->isConstantInitializer(Ctx, false));
+}

>From 07b4bf03a2b95be7e4de7c6b2fb0d09b89befc02 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rnk at google.com>
Date: Tue, 6 Feb 2024 00:25:59 +0000
Subject: [PATCH 3/6] Avoid emdash, use an ASCII hyphen

---
 clang/lib/AST/Expr.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 9190995c1df20..dec9e16ed7829 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3330,8 +3330,8 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   case InitListExprClass: {
     // C++ [temp.dep.expr]p2:
     //   The elements of an aggregate are:
-    //   — for an array, the array elements in increasing subscript order, or
-    //   — for a class, the direct base classes in declaration order, followed
+    //   - for an array, the array elements in increasing subscript order, or
+    //   - for a class, the direct base classes in declaration order, followed
     //     by the direct non-static data members (11.4) that are not members of
     //     an anonymous union, in declaration order.
     const InitListExpr *ILE = cast<InitListExpr>(this);

>From b75febc6e875943361c7cdf3e7274be22e04e278 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rnk at google.com>
Date: Tue, 6 Feb 2024 17:13:42 +0000
Subject: [PATCH 4/6] Fix formatting of unittests

---
 clang/unittests/AST/ASTExprTest.cpp | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/clang/unittests/AST/ASTExprTest.cpp b/clang/unittests/AST/ASTExprTest.cpp
index 257ac9c55d1bb..5ec6aea8edba3 100644
--- a/clang/unittests/AST/ASTExprTest.cpp
+++ b/clang/unittests/AST/ASTExprTest.cpp
@@ -28,12 +28,14 @@ using clang::tooling::buildASTFromCode;
 
 static IntegerLiteral *createIntLiteral(ASTContext &Ctx, uint32_t Value) {
   const int numBits = 32;
-  return IntegerLiteral::Create(Ctx, llvm::APInt(numBits, Value),
-                                Ctx.IntTy, {});
+  return IntegerLiteral::Create(Ctx, llvm::APInt(numBits, Value), Ctx.IntTy,
+                                {});
 }
 
-const CXXRecordDecl *getCXXRecordDeclNode(ASTUnit *AST, const std::string &Name) {
-  auto Result = match(cxxRecordDecl(hasName(Name)).bind("record"), AST->getASTContext());
+const CXXRecordDecl *getCXXRecordDeclNode(ASTUnit *AST,
+                                          const std::string &Name) {
+  auto Result =
+      match(cxxRecordDecl(hasName(Name)).bind("record"), AST->getASTContext());
   EXPECT_FALSE(Result.empty());
   return Result[0].getNodeAs<CXXRecordDecl>("record");
 }
@@ -89,9 +91,9 @@ TEST(ASTExpr, InitListIsConstantInitialized) {
   InitListExpr *BaseInit = new (Ctx) InitListExpr(Ctx, Loc, {}, Loc);
   BaseInit->setType(Ctx.getRecordType(Empty));
   Expr *Exprs[3] = {
-    BaseInit,
-    createIntLiteral(Ctx, 13),
-    createIntLiteral(Ctx, 42),
+      BaseInit,
+      createIntLiteral(Ctx, 13),
+      createIntLiteral(Ctx, 42),
   };
   InitListExpr *FooInit = new (Ctx) InitListExpr(Ctx, Loc, Exprs, Loc);
   FooInit->setType(Ctx.getRecordType(Foo));

>From 1975ac3431d484205d03ded751f9fa809927a0f4 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rnk at google.com>
Date: Thu, 8 Feb 2024 13:57:31 -0800
Subject: [PATCH 5/6] Update clang/lib/AST/Expr.cpp

Co-authored-by: Aaron Ballman <aaron at aaronballman.com>
---
 clang/lib/AST/Expr.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index dec9e16ed7829..8b10e28958326 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3328,7 +3328,7 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
            DIUE->getUpdater()->isConstantInitializer(Ctx, false, Culprit);
   }
   case InitListExprClass: {
-    // C++ [temp.dep.expr]p2:
+    // C++ [dcl.init.aggr]p2:
     //   The elements of an aggregate are:
     //   - for an array, the array elements in increasing subscript order, or
     //   - for a class, the direct base classes in declaration order, followed

>From d65576206f9e4f47bb44ba5e26f9eb8ec1da76c1 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rnk at google.com>
Date: Thu, 8 Feb 2024 22:04:50 +0000
Subject: [PATCH 6/6] update release notes

---
 clang/docs/ReleaseNotes.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 32440ee64e3eb..df3ad20f955ea 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -217,6 +217,10 @@ Bug Fixes to C++ Support
   Fixes (`#80971 ICE when explicit object parameter be a function parameter pack`)
 - Fixed a bug where abbreviated function templates would append their invented template parameters to
   an empty template parameter lists.
+- Clang now classifies aggregate initialization in C++17 and newer as constant
+  or non-constant more accurately. Previously, only a subset of the initializer
+  elements were considered, misclassifying some initializers as constant. Fixes
+  some of (`#80510 <https://github.com/llvm/llvm-project/issues/80510>`).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^



More information about the cfe-commits mailing list