[Clang][AST] {CXXDefaultArgExpr, CXXDefaultInitExpr}::hasRewrittenInit return true iff the init expression was really rebuild #99748
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…t return true iif the init expression was really rebuild Signed-off-by: yronglin <[email protected]>
yronglin
force-pushed
the
fix_default_init_rebuild
branch
from
a79b9bb to
c565183
Compare
yronglin
changed the title
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (yronglin) ChangesFull diff: https://github.com/llvm/llvm-project/pull/99748.diff 8 Files Affected:
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index c2feac525c1ea..e863bcb104c09 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -1277,7 +1277,8 @@ class CXXDefaultArgExpr final
DeclContext *UsedContext;
CXXDefaultArgExpr(StmtClass SC, SourceLocation Loc, ParmVarDecl *Param,
- Expr *RewrittenExpr, DeclContext *UsedContext)
+ DeclContext *UsedContext, Expr *RewrittenExpr,
+ bool HasRewrittenInit)
: Expr(SC,
Param->hasUnparsedDefaultArg()
? Param->getType().getNonReferenceType()
@@ -1286,7 +1287,7 @@ class CXXDefaultArgExpr final
Param->getDefaultArg()->getObjectKind()),
Param(Param), UsedContext(UsedContext) {
CXXDefaultArgExprBits.Loc = Loc;
- CXXDefaultArgExprBits.HasRewrittenInit = RewrittenExpr != nullptr;
+ CXXDefaultArgExprBits.HasRewrittenInit = HasRewrittenInit;
if (RewrittenExpr)
*getTrailingObjects<Expr *>() = RewrittenExpr;
setDependence(computeDependence(this));
@@ -1304,8 +1305,8 @@ class CXXDefaultArgExpr final
// \p Param is the parameter whose default argument is used by this
// expression.
static CXXDefaultArgExpr *Create(const ASTContext &C, SourceLocation Loc,
- ParmVarDecl *Param, Expr *RewrittenExpr,
- DeclContext *UsedContext);
+ ParmVarDecl *Param, DeclContext *UsedContext,
+ Expr *InitExpr, bool HasRewrittenInit);
// Retrieve the parameter that the argument was created from.
const ParmVarDecl *getParam() const { return Param; }
ParmVarDecl *getParam() { return Param; }
@@ -1385,7 +1386,7 @@ class CXXDefaultInitExpr final
CXXDefaultInitExpr(const ASTContext &Ctx, SourceLocation Loc,
FieldDecl *Field, QualType Ty, DeclContext *UsedContext,
- Expr *RewrittenInitExpr);
+ Expr *InitExpr, bool HasRewrittenInit);
CXXDefaultInitExpr(EmptyShell Empty, bool HasRewrittenInit)
: Expr(CXXDefaultInitExprClass, Empty) {
@@ -1399,7 +1400,7 @@ class CXXDefaultInitExpr final
/// by this expression.
static CXXDefaultInitExpr *Create(const ASTContext &Ctx, SourceLocation Loc,
FieldDecl *Field, DeclContext *UsedContext,
- Expr *RewrittenInitExpr);
+ Expr *InitExpr, bool HasRewrittenInit);
bool hasRewrittenInit() const {
return CXXDefaultInitExprBits.HasRewrittenInit;
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 0c27f6f5df2da..4465c78d05ea1 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -8107,8 +8107,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
RewrittenInit = ExprOrErr.get();
}
return CXXDefaultArgExpr::Create(Importer.getToContext(), *ToUsedLocOrErr,
- *ToParamOrErr, RewrittenInit,
- *UsedContextOrErr);
+ *ToParamOrErr, *UsedContextOrErr,
+ RewrittenInit, E->hasRewrittenInit());
}
ExpectedStmt
@@ -8814,7 +8814,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *E) {
}
return CXXDefaultInitExpr::Create(Importer.getToContext(), *ToBeginLocOrErr,
- ToField, *UsedContextOrErr, RewrittenInit);
+ ToField, *UsedContextOrErr, RewrittenInit,
+ E->hasRewrittenInit());
}
ExpectedStmt ASTNodeImporter::VisitCXXNamedCastExpr(CXXNamedCastExpr *E) {
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index 8d2a1b5611ccc..bbbca3a40aca9 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -1013,15 +1013,15 @@ CXXDefaultArgExpr *CXXDefaultArgExpr::CreateEmpty(const ASTContext &C,
return new (Mem) CXXDefaultArgExpr(EmptyShell(), HasRewrittenInit);
}
-CXXDefaultArgExpr *CXXDefaultArgExpr::Create(const ASTContext &C,
- SourceLocation Loc,
- ParmVarDecl *Param,
- Expr *RewrittenExpr,
- DeclContext *UsedContext) {
+CXXDefaultArgExpr *
+CXXDefaultArgExpr::Create(const ASTContext &C, SourceLocation Loc,
+ ParmVarDecl *Param, DeclContext *UsedContext,
+ Expr *RewrittenExpr, bool HasRewrittenInit) {
size_t Size = totalSizeToAlloc<Expr *>(RewrittenExpr != nullptr);
auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
- return new (Mem) CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param,
- RewrittenExpr, UsedContext);
+ return new (Mem)
+ CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param, UsedContext,
+ RewrittenExpr, HasRewrittenInit);
}
Expr *CXXDefaultArgExpr::getExpr() {
@@ -1042,7 +1042,7 @@ Expr *CXXDefaultArgExpr::getAdjustedRewrittenExpr() {
CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
SourceLocation Loc, FieldDecl *Field,
QualType Ty, DeclContext *UsedContext,
- Expr *RewrittenInitExpr)
+ Expr *InitExpr, bool HasRewrittenInit)
: Expr(CXXDefaultInitExprClass, Ty.getNonLValueExprType(Ctx),
Ty->isLValueReferenceType() ? VK_LValue
: Ty->isRValueReferenceType() ? VK_XValue
@@ -1050,10 +1050,10 @@ CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
/*FIXME*/ OK_Ordinary),
Field(Field), UsedContext(UsedContext) {
CXXDefaultInitExprBits.Loc = Loc;
- CXXDefaultInitExprBits.HasRewrittenInit = RewrittenInitExpr != nullptr;
+ CXXDefaultInitExprBits.HasRewrittenInit = HasRewrittenInit;
if (CXXDefaultInitExprBits.HasRewrittenInit)
- *getTrailingObjects<Expr *>() = RewrittenInitExpr;
+ *getTrailingObjects<Expr *>() = InitExpr;
assert(Field->hasInClassInitializer());
@@ -1067,16 +1067,15 @@ CXXDefaultInitExpr *CXXDefaultInitExpr::CreateEmpty(const ASTContext &C,
return new (Mem) CXXDefaultInitExpr(EmptyShell(), HasRewrittenInit);
}
-CXXDefaultInitExpr *CXXDefaultInitExpr::Create(const ASTContext &Ctx,
- SourceLocation Loc,
- FieldDecl *Field,
- DeclContext *UsedContext,
- Expr *RewrittenInitExpr) {
+CXXDefaultInitExpr *
+CXXDefaultInitExpr::Create(const ASTContext &Ctx, SourceLocation Loc,
+ FieldDecl *Field, DeclContext *UsedContext,
+ Expr *InitExpr, bool HasRewrittenInit) {
- size_t Size = totalSizeToAlloc<Expr *>(RewrittenInitExpr != nullptr);
+ size_t Size = totalSizeToAlloc<Expr *>(InitExpr != nullptr);
auto *Mem = Ctx.Allocate(Size, alignof(CXXDefaultInitExpr));
return new (Mem) CXXDefaultInitExpr(Ctx, Loc, Field, Field->getType(),
- UsedContext, RewrittenInitExpr);
+ UsedContext, InitExpr, HasRewrittenInit);
}
Expr *CXXDefaultInitExpr::getExpr() {
diff --git a/clang/lib/AST/JSONNodeDumper.cpp b/clang/lib/AST/JSONNodeDumper.cpp
index eeb314b8d32b0..305af8decca75 100644
--- a/clang/lib/AST/JSONNodeDumper.cpp
+++ b/clang/lib/AST/JSONNodeDumper.cpp
@@ -1579,11 +1579,11 @@ void JSONNodeDumper::VisitMaterializeTemporaryExpr(
}
void JSONNodeDumper::VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *Node) {
- attributeOnlyIfTrue("hasRewrittenInit", Node->hasRewrittenInit());
+ JOS.attribute("hasRewrittenInit", Node->hasRewrittenInit());
}
void JSONNodeDumper::VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *Node) {
- attributeOnlyIfTrue("hasRewrittenInit", Node->hasRewrittenInit());
+ JOS.attribute("hasRewrittenInit", Node->hasRewrittenInit());
}
void JSONNodeDumper::VisitCXXDependentScopeMemberExpr(
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8d24e34520e77..1abeece6f9371 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5426,6 +5426,7 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
bool InLifetimeExtendingContext = isInLifetimeExtendingContext();
+ bool HasRewrittenInit = false;
std::optional<ExpressionEvaluationContextRecord::InitializationContext>
InitializationContext =
OutermostDeclarationWithDelayedImmediateInvocations();
@@ -5458,10 +5459,10 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
ImmediateCallVisitor V(getASTContext());
if (!NestedDefaultChecking)
V.TraverseDecl(Param);
-
+ HasRewrittenInit = V.HasImmediateCalls || InLifetimeExtendingContext;
// Rewrite the call argument that was created from the corresponding
// parameter's default argument.
- if (V.HasImmediateCalls || InLifetimeExtendingContext) {
+ if (HasRewrittenInit) {
if (V.HasImmediateCalls)
ExprEvalContexts.back().DelayedDefaultInitializationContext = {
CallLoc, Param, CurContext};
@@ -5490,7 +5491,8 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
return ExprError();
return CXXDefaultArgExpr::Create(Context, InitializationContext->Loc, Param,
- Init, InitializationContext->Context);
+ InitializationContext->Context, Init,
+ HasRewrittenInit);
}
ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
@@ -5548,7 +5550,9 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
ImmediateCallVisitor V(getASTContext());
if (!NestedDefaultChecking)
V.TraverseDecl(Field);
- if (V.HasImmediateCalls) {
+
+ bool HasRewrittenInit = V.HasImmediateCalls;
+ if (HasRewrittenInit) {
ExprEvalContexts.back().DelayedDefaultInitializationContext = {Loc, Field,
CurContext};
ExprEvalContexts.back().IsCurrentlyCheckingDefaultArgumentOrInitializer =
@@ -5587,7 +5591,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
Field, InitializationContext->Context,
- Init);
+ Init, HasRewrittenInit);
}
// DR1351:
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 84e846356e437..35bbb9a7d1150 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -3369,9 +3369,10 @@ class TreeTransform {
/// require any semantic analysis. Subclasses may override this routine to
/// provide different behavior.
ExprResult RebuildCXXDefaultArgExpr(SourceLocation Loc, ParmVarDecl *Param,
- Expr *RewrittenExpr) {
+ Expr *InitExpr, bool HasRewrittenExpr) {
return CXXDefaultArgExpr::Create(getSema().Context, Loc, Param,
- RewrittenExpr, getSema().CurContext);
+ getSema().CurContext, InitExpr,
+ HasRewrittenExpr);
}
/// Build a new C++11 default-initialization expression.
@@ -13307,8 +13308,8 @@ TreeTransform<Derived>::TransformCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
InitRes.get() == E->getRewrittenExpr())
return E;
- return getDerived().RebuildCXXDefaultArgExpr(E->getUsedLocation(), Param,
- InitRes.get());
+ return getDerived().RebuildCXXDefaultArgExpr(
+ E->getUsedLocation(), Param, InitRes.get(), E->hasRewrittenInit());
}
template<typename Derived>
diff --git a/clang/test/AST/ast-dump-default-init-json.cpp b/clang/test/AST/ast-dump-default-init-json.cpp
index 1058b4e3ea4d9..9c800cfc23684 100644
--- a/clang/test/AST/ast-dump-default-init-json.cpp
+++ b/clang/test/AST/ast-dump-default-init-json.cpp
@@ -745,7 +745,7 @@ void test() {
// CHECK-NEXT: "qualType": "const A"
// CHECK-NEXT: },
// CHECK-NEXT: "valueCategory": "lvalue",
-// CHECK-NEXT: "hasRewrittenInit": true,
+// CHECK-NEXT: "hasRewrittenInit": false,
// CHECK-NEXT: "inner": [
// CHECK-NEXT: {
// CHECK-NEXT: "id": "0x{{.*}}",
diff --git a/clang/test/AST/ast-dump-default-init.cpp b/clang/test/AST/ast-dump-default-init.cpp
index 15b29f04bf21b..3703cfe4a8be6 100644
--- a/clang/test/AST/ast-dump-default-init.cpp
+++ b/clang/test/AST/ast-dump-default-init.cpp
@@ -11,7 +11,7 @@ struct B {
void test() {
B b{};
}
-// CHECK: -CXXDefaultInitExpr 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue has rewritten init
+// CHECK: -CXXDefaultInitExpr 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue
// CHECK-NEXT: `-ExprWithCleanups 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue
// CHECK-NEXT: `-MaterializeTemporaryExpr 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue extended by Field 0x{{[^ ]*}} 'a' 'const A &'
// CHECK-NEXT: `-ImplicitCastExpr 0x{{[^ ]*}} <{{.*}}> 'const A' <NoOp>
|
Sirraide
reviewed
Jul 26, 2024
|
Thanks for the review! Hmm, this patch was splitted from #91879. In that patch, we should guarantee the sub-expression was actually rebuilt when CXXDefaultInitExpr::hasRewrittenInit() returns true. But #91879 and the dependency patch #87933 was reverted, so I'd like to implement this in a separate patch. Reapply CWG1815 patch was used to reapply CWG1815, and I'll also reapply #91879 once CWG1815 landed. I'd like to preserve the history of my commit in Reapply CWG1815 patch patch to make it's easily to review. Reapply CWG1815 patch still needs more improve, but I will finish it as soon as possible. |
Sirraide
reviewed
Jul 26, 2024
Comment on lines
+1019
to
1020
| Expr *RewrittenExpr, bool HasRewrittenInit) { | ||
| size_t Size = totalSizeToAlloc<Expr *>(RewrittenExpr != nullptr); |
There was a problem hiding this comment.
Maybe the naming here is just weird, but is there ever a case where RewrittenExpr != nullptr and HasRewrittenInit don’t evaluate to the same value? Because in that case, either HasRewrittenInit seems superfluous or RewrittenExpr is badly named, because a ‘non-rewritten RewrittenExpr’ sounds a bit nonsensical...
There was a problem hiding this comment.
I have the same question!
In particular in the test change, I think the expression is rewritten.
shafik
reviewed
Jul 26, 2024
| @@ -1579,11 +1579,11 @@ void JSONNodeDumper::VisitMaterializeTemporaryExpr( | |||
| } | |||
|
|
|||
| void JSONNodeDumper::VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *Node) { | |||
| attributeOnlyIfTrue("hasRewrittenInit", Node->hasRewrittenInit()); | |||
| JOS.attribute("hasRewrittenInit", Node->hasRewrittenInit()); | |||
There was a problem hiding this comment.
So if IIUC this used to only write the attribute if the value was true, so we should have a test for both cases true and false to verify this is working as intended.
|
Sorry for the very late reply! I have mentioned this change in #117437, could you please help review it? I'll close this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.