[clang] [Clang] Correctly handle allocations in the condition of a `if constexpr` (PR #146890)
Corentin Jabot via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 4 00:11:56 PDT 2025
https://github.com/cor3ntin updated https://github.com/llvm/llvm-project/pull/146890
>From c138801c212f69f4dee83c7516e0e77eb876a9f0 Mon Sep 17 00:00:00 2001
From: Corentin Jabot <corentinjabot at gmail.com>
Date: Thu, 3 Jul 2025 14:26:59 +0200
Subject: [PATCH 1/4] [Clang] Correctly handle allocations in the condition of
a `if constexpr`
Deal with the following scenario
```cpp
struct S {
char* c = new char;
constexpr ~S() {
delete c;
}
};
if constexpr((S{}, true)){};
```
There were two issues
- We need to produce a full expressions _before_ evaluating the condition
(otherwise automatic variables are neber destroyed)
- We need to preserve the evaluation context of the condition while doing
the semantics analysis for it (lest it is evaluated in a non constant
evaluated context)
Fixes #120197
Fixes #134820
---
clang/docs/ReleaseNotes.rst | 1 +
clang/include/clang/Sema/Sema.h | 6 ++--
clang/lib/Parse/ParseExprCXX.cpp | 16 ++++-----
clang/lib/Sema/SemaExpr.cpp | 11 ++++---
clang/lib/Sema/SemaExprCXX.cpp | 9 ++++-
.../test/SemaCXX/cxx2a-constexpr-dynalloc.cpp | 33 +++++++++++++++++++
6 files changed, 58 insertions(+), 18 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3d893e0aa8e2c..9f9fc84b53104 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -896,6 +896,7 @@ Bug Fixes to C++ Support
- Fixed a crash when constant evaluating some explicit object member assignment operators. (#GH142835)
- Fixed an access checking bug when substituting into concepts (#GH115838)
- Fix a bug where private access specifier of overloaded function not respected. (#GH107629)
+- Correctly handle allocations in the condition of a ``if constexpr``.(#GH120197) (#GH134820)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 3fe26f950ad51..b281b1cfef96a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7723,12 +7723,12 @@ class Sema final : public SemaBase {
class ConditionResult {
Decl *ConditionVar;
- FullExprArg Condition;
+ ExprResult Condition;
bool Invalid;
std::optional<bool> KnownValue;
friend class Sema;
- ConditionResult(Sema &S, Decl *ConditionVar, FullExprArg Condition,
+ ConditionResult(Sema &S, Decl *ConditionVar, ExprResult Condition,
bool IsConstexpr)
: ConditionVar(ConditionVar), Condition(Condition), Invalid(false) {
if (IsConstexpr && Condition.get()) {
@@ -7739,7 +7739,7 @@ class Sema final : public SemaBase {
}
}
explicit ConditionResult(bool Invalid)
- : ConditionVar(nullptr), Condition(nullptr), Invalid(Invalid),
+ : ConditionVar(nullptr), Condition(Invalid), Invalid(Invalid),
KnownValue(std::nullopt) {}
public:
diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp
index 1ea0cf52933f6..9f36c3ade5e74 100644
--- a/clang/lib/Parse/ParseExprCXX.cpp
+++ b/clang/lib/Parse/ParseExprCXX.cpp
@@ -1931,15 +1931,13 @@ Parser::ParseCXXCondition(StmtResult *InitStmt, SourceLocation Loc,
return ParseCXXCondition(nullptr, Loc, CK, MissingOK);
}
- ExprResult Expr = [&] {
- EnterExpressionEvaluationContext Eval(
- Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
- /*LambdaContextDecl=*/nullptr,
- /*ExprContext=*/Sema::ExpressionEvaluationContextRecord::EK_Other,
- /*ShouldEnter=*/CK == Sema::ConditionKind::ConstexprIf);
- // Parse the expression.
- return ParseExpression(); // expression
- }();
+ EnterExpressionEvaluationContext Eval(
+ Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+ /*LambdaContextDecl=*/nullptr,
+ /*ExprContext=*/Sema::ExpressionEvaluationContextRecord::EK_Other,
+ /*ShouldEnter=*/CK == Sema::ConditionKind::ConstexprIf);
+
+ ExprResult Expr = ParseExpression();
if (Expr.isInvalid())
return Sema::ConditionError();
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 437df742d572b..7c65489cc6234 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -20629,6 +20629,9 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
case ConditionKind::ConstexprIf:
Cond = CheckBooleanCondition(Loc, SubExpr, true);
+ assert(isa<FullExpr>(Cond.get()) &&
+ "we should have converted this expression to a FullExpr before "
+ "evaluating it");
break;
case ConditionKind::Switch:
@@ -20641,12 +20644,10 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
if (!Cond.get())
return ConditionError();
}
- // FIXME: FullExprArg doesn't have an invalid bit, so check nullness instead.
- FullExprArg FullExpr = MakeFullExpr(Cond.get(), Loc);
- if (!FullExpr.get())
- return ConditionError();
+ if (!isa<FullExpr>(Cond.get()))
+ Cond = ActOnFinishFullExpr(Cond.get(), Loc, /*DiscardedValue*/ false);
- return ConditionResult(*this, nullptr, FullExpr,
+ return ConditionResult(*this, nullptr, Cond,
CK == ConditionKind::ConstexprIf);
}
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 4a86cbd0633b6..f17a338825423 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -4357,7 +4357,8 @@ Sema::ConditionResult Sema::ActOnConditionVariable(Decl *ConditionVar,
CheckConditionVariable(cast<VarDecl>(ConditionVar), StmtLoc, CK);
if (E.isInvalid())
return ConditionError();
- return ConditionResult(*this, ConditionVar, MakeFullExpr(E.get(), StmtLoc),
+ E = ActOnFinishFullExpr(E.get(), /*DiscardedValue*/ false);
+ return ConditionResult(*this, ConditionVar, E,
CK == ConditionKind::ConstexprIf);
}
@@ -4417,6 +4418,12 @@ ExprResult Sema::CheckCXXBooleanCondition(Expr *CondExpr, bool IsConstexpr) {
if (!IsConstexpr || E.isInvalid() || E.get()->isValueDependent())
return E;
+ E = ActOnFinishFullExpr(E.get(), E.get()->getExprLoc(),
+ /*DiscardedValue*/ false,
+ /*IsConstexpr*/ true);
+ if (E.isInvalid())
+ return E;
+
// FIXME: Return this value to the caller so they don't need to recompute it.
llvm::APSInt Cond;
E = VerifyIntegerConstantExpression(
diff --git a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
index ed8cbbbfe7067..edffe177e08c7 100644
--- a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
+++ b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
@@ -242,3 +242,36 @@ void f() {
}
}
+
+namespace GH134820 {
+struct S {
+ char* c = new char;
+ constexpr ~S() {
+ delete c;
+ }
+};
+
+int f() {
+ if constexpr((S{}, true)) {
+ return 1;
+ }
+ return 0;
+}
+}
+
+namespace GH120197{
+struct NonTrivialDtor {
+ NonTrivialDtor() = default;
+ NonTrivialDtor(const NonTrivialDtor&) = default;
+ NonTrivialDtor(NonTrivialDtor&&) = default;
+ NonTrivialDtor& operator=(const NonTrivialDtor&) = default;
+ NonTrivialDtor& operator=(NonTrivialDtor&&) = default;
+ constexpr ~NonTrivialDtor() noexcept {}
+};
+
+static_assert(((void)NonTrivialDtor{}, true)); // passes
+
+void f() {
+ if constexpr ((void)NonTrivialDtor{}, true) {}
+}
+}
>From 7d6b68a4a6d1cc6356d34dd60ad513de9b2a78bc Mon Sep 17 00:00:00 2001
From: Corentin Jabot <corentinjabot at gmail.com>
Date: Thu, 3 Jul 2025 17:00:39 +0200
Subject: [PATCH 2/4] add tests and remove bogus assert
---
clang/lib/Sema/SemaExpr.cpp | 10 +++++-----
clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp | 8 +++++++-
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 7c65489cc6234..1a352fbcb3cbb 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -20628,10 +20628,8 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
break;
case ConditionKind::ConstexprIf:
+ // Note: this might produce a FullExpr
Cond = CheckBooleanCondition(Loc, SubExpr, true);
- assert(isa<FullExpr>(Cond.get()) &&
- "we should have converted this expression to a FullExpr before "
- "evaluating it");
break;
case ConditionKind::Switch:
@@ -20643,10 +20641,12 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
{SubExpr}, PreferredConditionType(CK));
if (!Cond.get())
return ConditionError();
- }
- if (!isa<FullExpr>(Cond.get()))
+ } else if (Cond.isUsable() && !isa<FullExpr>(Cond.get()))
Cond = ActOnFinishFullExpr(Cond.get(), Loc, /*DiscardedValue*/ false);
+ if (Cond.isInvalid())
+ return ConditionError();
+
return ConditionResult(*this, nullptr, Cond,
CK == ConditionKind::ConstexprIf);
}
diff --git a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
index edffe177e08c7..606e06957c4ea 100644
--- a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
+++ b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
@@ -252,7 +252,13 @@ struct S {
};
int f() {
- if constexpr((S{}, true)) {
+ if constexpr((S{}, true)) { // expected-warning{{left operand of comma operator has no effect}}
+ return 1;
+ }
+ if constexpr(S s; (s, true)) { // expected-warning{{left operand of comma operator has no effect}}
+ return 1;
+ }
+ if constexpr(S s; (s, true)) { // expected-warning{{left operand of comma operator has no effect}}
return 1;
}
return 0;
>From b26b29d16f9aa3cbdd6d2767b7897d363fd232d5 Mon Sep 17 00:00:00 2001
From: Corentin Jabot <corentinjabot at gmail.com>
Date: Thu, 3 Jul 2025 17:06:23 +0200
Subject: [PATCH 3/4] more tests
---
clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
index 606e06957c4ea..f01cf69385576 100644
--- a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
+++ b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
@@ -249,6 +249,7 @@ struct S {
constexpr ~S() {
delete c;
}
+ int i = 0;
};
int f() {
@@ -261,6 +262,9 @@ int f() {
if constexpr(S s; (s, true)) { // expected-warning{{left operand of comma operator has no effect}}
return 1;
}
+ if constexpr(constexpr int _ = S{}.i; true) {
+ return 1;
+ }
return 0;
}
}
>From c7a10efe30c406efd62d8048ce4d35980bc32412 Mon Sep 17 00:00:00 2001
From: Corentin Jabot <corentinjabot at gmail.com>
Date: Fri, 4 Jul 2025 09:11:40 +0200
Subject: [PATCH 4/4] correctly handle dependent conditions
---
clang/lib/Sema/SemaExpr.cpp | 2 +-
clang/lib/Sema/TreeTransform.h | 7 ++++++
.../test/SemaCXX/cxx2a-constexpr-dynalloc.cpp | 23 ++++++++++++++++++-
3 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 1a352fbcb3cbb..4f3936e39717b 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -20644,7 +20644,7 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
} else if (Cond.isUsable() && !isa<FullExpr>(Cond.get()))
Cond = ActOnFinishFullExpr(Cond.get(), Loc, /*DiscardedValue*/ false);
- if (Cond.isInvalid())
+ if (!Cond.isUsable())
return ConditionError();
return ConditionResult(*this, nullptr, Cond,
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 0d58587cb8a99..3996f9b2c0b5f 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -4561,6 +4561,13 @@ bool TreeTransform<Derived>::TransformExprs(Expr *const *Inputs,
template <typename Derived>
Sema::ConditionResult TreeTransform<Derived>::TransformCondition(
SourceLocation Loc, VarDecl *Var, Expr *Expr, Sema::ConditionKind Kind) {
+
+ EnterExpressionEvaluationContext Eval(
+ SemaRef, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+ /*LambdaContextDecl=*/nullptr,
+ /*ExprContext=*/Sema::ExpressionEvaluationContextRecord::EK_Other,
+ /*ShouldEnter=*/Kind == Sema::ConditionKind::ConstexprIf);
+
if (Var) {
VarDecl *ConditionVar = cast_or_null<VarDecl>(
getDerived().TransformDefinition(Var->getLocation(), Var));
diff --git a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
index f01cf69385576..c5d5427170394 100644
--- a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
+++ b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
@@ -256,7 +256,7 @@ int f() {
if constexpr((S{}, true)) { // expected-warning{{left operand of comma operator has no effect}}
return 1;
}
- if constexpr(S s; (s, true)) { // expected-warning{{left operand of comma operator has no effect}}
+ if constexpr(S s; (S{}, true)) { // expected-warning{{left operand of comma operator has no effect}}
return 1;
}
if constexpr(S s; (s, true)) { // expected-warning{{left operand of comma operator has no effect}}
@@ -267,8 +267,29 @@ int f() {
}
return 0;
}
+
+template <typename T>
+int f2() {
+ if constexpr((T{}, true)) { // expected-warning{{left operand of comma operator has no effect}}
+ return 1;
+ }
+ if constexpr(T s; (T{}, true)) { // expected-warning{{left operand of comma operator has no effect}}
+ return 1;
+ }
+ if constexpr(T s; (s, true)) { // expected-warning{{left operand of comma operator has no effect}}
+ return 1;
+ }
+ if constexpr(constexpr int _ = T{}.i; true) {
+ return 1;
+ }
+ return 0;
+}
+
+void test() {
+ f2<S>(); // expected-note {{in instantiation}}
}
+}
namespace GH120197{
struct NonTrivialDtor {
NonTrivialDtor() = default;
More information about the cfe-commits
mailing list