Skip to content

Consider aggregate bases when checking if an InitListExpr is constant #80519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 8, 2024
Merged

Consider aggregate bases when checking if an InitListExpr is constant #80519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ab5ba3
wip
rnk Feb 3, 2024
c630eee
update comments, run test in past language modes, and add unit test
rnk Feb 5, 2024
07b4bf0
Avoid emdash, use an ASCII hyphen
rnk Feb 6, 2024
b75febc
Fix formatting of unittests
rnk Feb 6, 2024
1975ac3
Update clang/lib/AST/Expr.cpp
rnk Feb 8, 2024
d74fc2f
Merge branch 'main' into check-cxx17-bases-constant
rnk Feb 8, 2024
d655762
update release notes
rnk Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions clang/lib/AST/Expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be checking CPlusPlus17 here?

Also I would like to see a standard quote in the comments. I know we are not doing this locally but these are really helpful and we should be doing this more consistently.

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)
Expand Down
20 changes: 20 additions & 0 deletions clang/test/SemaCXX/compound-literal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcc accepts this: https://godbolt.org/z/enWrG56je

Why do we believe this should be rejected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct and I did discover that in my testing, but I think fixing that goes beyond the scope of this patch. Clang currently rejects this case if you remove the empty base: https://godbolt.org/z/1x8hshM75 Adding support for non-constant compound literals at file scope requires codegen changes, and this is fixing a small bug to make more isConstantInitializer correct.

The real issue is that I couldn't find a better way to test isConstantInitializer that will be resilient to that future bug fix, and I'm not sure what to do about that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and make a targeted unit test for isConstantInitializer to see if that is nicer

};
}
#endif