[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 13:57:39 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/5] 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/5] 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/5] 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/5] 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/5] 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
More information about the cfe-commits
mailing list