<div dir="ltr">Thanks, fixed and re-committed as 00d3e6c1b4d0b7879afc6002b721111b49ecf755.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 6 Oct 2020 at 06:50, Dmitri Gribenko via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Author: Dmitri Gribenko<br>
Date: 2020-10-06T15:49:44+02:00<br>
New Revision: 37c74dfe72ecf4e7def22702c5a944682a7865df<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/37c74dfe72ecf4e7def22702c5a944682a7865df" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/37c74dfe72ecf4e7def22702c5a944682a7865df</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/37c74dfe72ecf4e7def22702c5a944682a7865df.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/37c74dfe72ecf4e7def22702c5a944682a7865df.diff</a><br>
<br>
LOG: Revert "[c++17] Implement P0145R3 during constant evaluation."<br>
<br>
This reverts commit ded79be63555f4e5bfdb0db27ef22b71fe568474. It causes<br>
a crash (I sent the crash reproducer directly to the author).<br>
<br>
Added: <br>
<br>
<br>
Modified: <br>
clang/lib/AST/ExprConstant.cpp<br>
clang/test/SemaCXX/constant-expression-cxx1z.cpp<br>
clang/www/cxx_status.html<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp<br>
index 49ad01f27545..4460e3a17e6d 100644<br>
--- a/clang/lib/AST/ExprConstant.cpp<br>
+++ b/clang/lib/AST/ExprConstant.cpp<br>
@@ -1856,12 +1856,8 @@ void CallStackFrame::describe(raw_ostream &Out) {<br>
Out << ", ";<br>
<br>
const ParmVarDecl *Param = *I;<br>
- if (Arguments) {<br>
- const APValue &Arg = Arguments[ArgIndex];<br>
- Arg.printPretty(Out, Info.Ctx, Param->getType());<br>
- } else {<br>
- Out << "<...>";<br>
- }<br>
+ const APValue &Arg = Arguments[ArgIndex];<br>
+ Arg.printPretty(Out, Info.Ctx, Param->getType());<br>
<br>
if (ArgIndex == 0 && IsMemberCall)<br>
Out << "->" << *Callee << '(';<br>
@@ -5796,8 +5792,6 @@ typedef SmallVector<APValue, 8> ArgVector;<br>
/// EvaluateArgs - Evaluate the arguments to a function call.<br>
static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,<br>
EvalInfo &Info, const FunctionDecl *Callee) {<br>
- ArgValues.resize(Args.size());<br>
-<br>
bool Success = true;<br>
llvm::SmallBitVector ForbiddenNullArgs;<br>
if (Callee->hasAttr<NonNullAttr>()) {<br>
@@ -5815,6 +5809,8 @@ static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,<br>
}<br>
}<br>
}<br>
+ // FIXME: This is the wrong evaluation order for an assignment operator<br>
+ // called via operator syntax.<br>
for (unsigned Idx = 0; Idx < Args.size(); Idx++) {<br>
if (!Evaluate(ArgValues[Idx], Info, Args[Idx])) {<br>
// If we're checking for a potential constant expression, evaluate all<br>
@@ -5838,13 +5834,17 @@ static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,<br>
/// Evaluate a function call.<br>
static bool HandleFunctionCall(SourceLocation CallLoc,<br>
const FunctionDecl *Callee, const LValue *This,<br>
- ArrayRef<const Expr *> Args, APValue *ArgValues,<br>
- const Stmt *Body, EvalInfo &Info,<br>
- APValue &Result, const LValue *ResultSlot) {<br>
+ ArrayRef<const Expr*> Args, const Stmt *Body,<br>
+ EvalInfo &Info, APValue &Result,<br>
+ const LValue *ResultSlot) {<br>
+ ArgVector ArgValues(Args.size());<br>
+ if (!EvaluateArgs(Args, ArgValues, Info, Callee))<br>
+ return false;<br>
+<br>
if (!Info.CheckCallLimit(CallLoc))<br>
return false;<br>
<br>
- CallStackFrame Frame(Info, CallLoc, Callee, This, ArgValues);<br>
+ CallStackFrame Frame(Info, CallLoc, Callee, This, ArgValues.data());<br>
<br>
// For a trivial copy or move assignment, perform an APValue copy. This is<br>
// essential for unions, where the operations performed by the assignment<br>
@@ -7293,8 +7293,6 @@ class ExprEvaluatorBase<br>
auto Args = llvm::makeArrayRef(E->getArgs(), E->getNumArgs());<br>
bool HasQualifier = false;<br>
<br>
- ArgVector ArgValues;<br>
-<br>
// Extract function decl and 'this' pointer from the callee.<br>
if (CalleeType->isSpecificBuiltinType(BuiltinType::BoundMember)) {<br>
const CXXMethodDecl *Member = nullptr;<br>
@@ -7343,22 +7341,6 @@ class ExprEvaluatorBase<br>
return Error(E);<br>
}<br>
<br>
- // For an (overloaded) assignment expression, evaluate the RHS before the<br>
- // LHS.<br>
- auto *OCE = dyn_cast<CXXOperatorCallExpr>(E);<br>
- if (OCE && OCE->isAssignmentOp()) {<br>
- assert(Args.size() == 2 && "wrong number of arguments in assignment");<br>
- if (isa<CXXMethodDecl>(FD)) {<br>
- // Args[0] is the object argument.<br>
- if (!EvaluateArgs({Args[1]}, ArgValues, Info, FD))<br>
- return false;<br>
- } else {<br>
- if (!EvaluateArgs({Args[1], Args[0]}, ArgValues, Info, FD))<br>
- return false;<br>
- std::swap(ArgValues[0], ArgValues[1]);<br>
- }<br>
- }<br>
-<br>
// Overloaded operator calls to member functions are represented as normal<br>
// calls with '*this' as the first argument.<br>
const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD);<br>
@@ -7421,11 +7403,6 @@ class ExprEvaluatorBase<br>
} else<br>
return Error(E);<br>
<br>
- // Evaluate the arguments now if we've not already done so.<br>
- if (ArgValues.empty() && !Args.empty() &&<br>
- !EvaluateArgs(Args, ArgValues, Info, FD))<br>
- return false;<br>
-<br>
SmallVector<QualType, 4> CovariantAdjustmentPath;<br>
if (This) {<br>
auto *NamedMember = dyn_cast<CXXMethodDecl>(FD);<br>
@@ -7447,7 +7424,6 @@ class ExprEvaluatorBase<br>
// Destructor calls are <br>
diff erent enough that they have their own codepath.<br>
if (auto *DD = dyn_cast<CXXDestructorDecl>(FD)) {<br>
assert(This && "no 'this' pointer for destructor call");<br>
- assert(ArgValues.empty() && "unexpected destructor arguments");<br>
return HandleDestruction(Info, E, *This,<br>
Info.Ctx.getRecordType(DD->getParent()));<br>
}<br>
@@ -7456,8 +7432,8 @@ class ExprEvaluatorBase<br>
Stmt *Body = FD->getBody(Definition);<br>
<br>
if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body) ||<br>
- !HandleFunctionCall(E->getExprLoc(), Definition, This, Args,<br>
- ArgValues.data(), Body, Info, Result, ResultSlot))<br>
+ !HandleFunctionCall(E->getExprLoc(), Definition, This, Args, Body, Info,<br>
+ Result, ResultSlot))<br>
return false;<br>
<br>
if (!CovariantAdjustmentPath.empty() &&<br>
@@ -8095,20 +8071,17 @@ bool LValueExprEvaluator::VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {<br>
if (E->getBase()->getType()->isVectorType())<br>
return Error(E);<br>
<br>
- APSInt Index;<br>
bool Success = true;<br>
-<br>
- // C++17's rules require us to evaluate the LHS first, regardless of which<br>
- // side is the base.<br>
- for (const Expr *SubExpr : {E->getLHS(), E->getRHS()}) {<br>
- if (SubExpr == E->getBase() ? !evaluatePointer(SubExpr, Result)<br>
- : !EvaluateInteger(SubExpr, Index, Info)) {<br>
- if (!Info.noteFailure())<br>
- return false;<br>
- Success = false;<br>
- }<br>
+ if (!evaluatePointer(E->getBase(), Result)) {<br>
+ if (!Info.noteFailure())<br>
+ return false;<br>
+ Success = false;<br>
}<br>
<br>
+ APSInt Index;<br>
+ if (!EvaluateInteger(E->getIdx(), Index, Info))<br>
+ return false;<br>
+<br>
return Success &&<br>
HandleLValueArrayAdjustment(Info, E, Result, E->getType(), Index);<br>
}<br>
@@ -8152,10 +8125,7 @@ bool LValueExprEvaluator::VisitCompoundAssignOperator(<br>
if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())<br>
return Error(CAO);<br>
<br>
- // C++17 onwards require that we evaluate the RHS first.<br>
APValue RHS;<br>
- if (!Evaluate(RHS, this->Info, CAO->getRHS()))<br>
- return false;<br>
<br>
// The overall lvalue result is the result of evaluating the LHS.<br>
if (!this->Visit(CAO->getLHS())) {<br>
@@ -8164,6 +8134,9 @@ bool LValueExprEvaluator::VisitCompoundAssignOperator(<br>
return false;<br>
}<br>
<br>
+ if (!Evaluate(RHS, this->Info, CAO->getRHS()))<br>
+ return false;<br>
+<br>
return handleCompoundAssignment(<br>
this->Info, CAO,<br>
Result, CAO->getLHS()->getType(), CAO->getComputationLHSType(),<br>
@@ -8174,10 +8147,7 @@ bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {<br>
if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())<br>
return Error(E);<br>
<br>
- // C++17 onwards require that we evaluate the RHS first.<br>
APValue NewVal;<br>
- if (!Evaluate(NewVal, this->Info, E->getRHS()))<br>
- return false;<br>
<br>
if (!this->Visit(E->getLHS())) {<br>
if (Info.noteFailure())<br>
@@ -8185,6 +8155,9 @@ bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {<br>
return false;<br>
}<br>
<br>
+ if (!Evaluate(NewVal, this->Info, E->getRHS()))<br>
+ return false;<br>
+<br>
if (Info.getLangOpts().CPlusPlus20 &&<br>
!HandleUnionActiveMemberChange(Info, E->getLHS(), Result))<br>
return false;<br>
@@ -15297,8 +15270,7 @@ bool Expr::isPotentialConstantExpr(const FunctionDecl *FD,<br>
} else {<br>
SourceLocation Loc = FD->getLocation();<br>
HandleFunctionCall(Loc, FD, (MD && MD->isInstance()) ? &This : nullptr,<br>
- Args, /*ArgValues*/ nullptr, FD->getBody(), Info,<br>
- Scratch, nullptr);<br>
+ Args, FD->getBody(), Info, Scratch, nullptr);<br>
}<br>
<br>
return Diags.empty();<br>
@@ -15320,8 +15292,13 @@ bool Expr::isPotentialConstantExprUnevaluated(Expr *E,<br>
Info.CheckingPotentialConstantExpression = true;<br>
<br>
// Fabricate a call stack frame to give the arguments a plausible cover story.<br>
- CallStackFrame Frame(Info, SourceLocation(), FD, /*This*/ nullptr,<br>
- /*ArgValues*/ nullptr);<br>
+ ArrayRef<const Expr*> Args;<br>
+ ArgVector ArgValues(0);<br>
+ bool Success = EvaluateArgs(Args, ArgValues, Info, FD);<br>
+ (void)Success;<br>
+ assert(Success &&<br>
+ "Failed to set up arguments for potential constant evaluation");<br>
+ CallStackFrame Frame(Info, SourceLocation(), FD, nullptr, ArgValues.data());<br>
<br>
APValue ResultScratch;<br>
Evaluate(ResultScratch, Info, E);<br>
<br>
diff --git a/clang/test/SemaCXX/constant-expression-cxx1z.cpp b/clang/test/SemaCXX/constant-expression-cxx1z.cpp<br>
index 7770e92c6331..2b366adf2e91 100644<br>
--- a/clang/test/SemaCXX/constant-expression-cxx1z.cpp<br>
+++ b/clang/test/SemaCXX/constant-expression-cxx1z.cpp<br>
@@ -59,112 +59,3 @@ void test() {<br>
else if constexpr (v<int>) {}<br>
}<br>
}<br>
-<br>
-// Check that assignment operators evaluate their operands right-to-left.<br>
-namespace EvalOrder {<br>
- template<typename T> struct lvalue {<br>
- T t;<br>
- constexpr T &get() { return t; }<br>
- };<br>
-<br>
- struct UserDefined {<br>
- int n = 0;<br>
- constexpr UserDefined &operator=(const UserDefined&) { return *this; }<br>
- constexpr UserDefined &operator+=(const UserDefined&) { return *this; }<br>
- constexpr void operator<<(const UserDefined&) const {}<br>
- constexpr void operator>>(const UserDefined&) const {}<br>
- constexpr void operator+(const UserDefined&) const {}<br>
- constexpr void operator[](int) const {}<br>
- };<br>
- constexpr UserDefined ud;<br>
-<br>
- struct NonMember {};<br>
- constexpr void operator+=(NonMember, NonMember) {}<br>
- constexpr void operator<<(NonMember, NonMember) {}<br>
- constexpr void operator>>(NonMember, NonMember) {}<br>
- constexpr void operator+(NonMember, NonMember) {}<br>
- constexpr NonMember nm;<br>
-<br>
- constexpr void f(...) {}<br>
-<br>
- // Helper to ensure that 'a' is evaluated before 'b'.<br>
- struct seq_checker {<br>
- bool done_a = false;<br>
- bool done_b = false;<br>
-<br>
- template <typename T> constexpr T &&a(T &&v) {<br>
- done_a = true;<br>
- return (T &&)v;<br>
- }<br>
- template <typename T> constexpr T &&b(T &&v) {<br>
- if (!done_a)<br>
- throw "wrong";<br>
- done_b = true;<br>
- return (T &&)v;<br>
- }<br>
-<br>
- constexpr bool ok() { return done_a && done_b; }<br>
- };<br>
-<br>
- // SEQ(expr), where part of the expression is tagged A(...) and part is<br>
- // tagged B(...), checks that A is evaluated before B.<br>
- #define A sc.a<br>
- #define B sc.b<br>
- #define SEQ(...) static_assert([](seq_checker sc) { void(__VA_ARGS__); return sc.ok(); }({}))<br>
-<br>
- // Longstanding sequencing rules.<br>
- SEQ((A(1), B(2)));<br>
- SEQ((A(true) ? B(2) : throw "huh?"));<br>
- SEQ((A(false) ? throw "huh?" : B(2)));<br>
- SEQ(A(true) && B(true));<br>
- SEQ(A(false) || B(true));<br>
-<br>
- // From P0145R3:<br>
-<br>
- // Rules 1 and 2 have no effect ('b' is not an expression).<br>
-<br>
- // Rule 3: a->*b<br>
- SEQ(A(ud).*B(&UserDefined::n));<br>
- SEQ(A(&ud)->*B(&UserDefined::n));<br>
-<br>
- // Rule 4: a(b1, b2, b3)<br>
- SEQ(A(f)(B(1), B(2), B(3)));<br>
-<br>
- // Rule 5: b = a, b @= a<br>
- SEQ(B(lvalue<int>().get()) = A(0));<br>
- SEQ(B(lvalue<UserDefined>().get()) = A(ud));<br>
- SEQ(B(lvalue<int>().get()) += A(0));<br>
- SEQ(B(lvalue<UserDefined>().get()) += A(ud));<br>
- SEQ(B(lvalue<NonMember>().get()) += A(nm));<br>
-<br>
- // Rule 6: a[b]<br>
- constexpr int arr[3] = {};<br>
- SEQ(A(arr)[B(0)]);<br>
- SEQ(A(+arr)[B(0)]);<br>
- SEQ(A(0)[B(arr)]);<br>
- SEQ(A(0)[B(+arr)]);<br>
- SEQ(A(ud)[B(0)]);<br>
-<br>
- // Rule 7: a << b<br>
- SEQ(A(1) << B(2));<br>
- SEQ(A(ud) << B(ud));<br>
- SEQ(A(nm) << B(nm));<br>
-<br>
- // Rule 8: a >> b<br>
- SEQ(A(1) >> B(2));<br>
- SEQ(A(ud) >> B(ud));<br>
- SEQ(A(nm) >> B(nm));<br>
-<br>
- // No particular order of evaluation is specified in other cases, but we in<br>
- // practice evaluate left-to-right.<br>
- // FIXME: Technically we're expected to check for undefined behavior due to<br>
- // unsequenced read and modification and treat it as non-constant due to UB.<br>
- SEQ(A(1) + B(2));<br>
- SEQ(A(ud) + B(ud));<br>
- SEQ(A(nm) + B(nm));<br>
- SEQ(f(A(1), B(2)));<br>
-<br>
- #undef SEQ<br>
- #undef A<br>
- #undef B<br>
-}<br>
<br>
diff --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html<br>
index 9c39e396edd4..3c546eb409de 100755<br>
--- a/clang/www/cxx_status.html<br>
+++ b/clang/www/cxx_status.html<br>
@@ -807,7 +807,6 @@ <h2 id="cxx17">C++17 implementation status</h2><br>
<tt>operator&&</tt>, <tt>operator||</tt>, and <tt>operator,</tt><br>
functions using expression syntax are no longer guaranteed to be destroyed in<br>
reverse construction order in that ABI.<br>
-This is not fully supported during constant expression evaluation until Clang 12.<br>
</span><br><br>
<span id="p0522">(10): Despite being the resolution to a Defect Report, this<br>
feature is disabled by default in all language versions, and can be enabled<br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>