From 6c29073efb0c22045868bc3622f8fc27f43fca41 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 12 May 2020 13:14:32 -0700 Subject: [PATCH] PR45589: Properly decompose overloaded `&&` and `||` operators in constraint expressions. We create overloaded `&&` and `||` operators to hold the possible unqualified lookup results (if any) when the operands are dependent. We could avoid building these in some cases (we will never use the stored lookup results, and it would be better to not store them or perform the lookups), but in the general case we will probably still need to handle overloaded operators even with that optimization. --- clang/include/clang/Sema/Sema.h | 7 +- clang/lib/Sema/SemaConcept.cpp | 121 +++++++++++++++++++------------- clang/lib/Sema/SemaExpr.cpp | 5 -- clang/test/SemaTemplate/constraints.cpp | 26 +++++++ 4 files changed, 98 insertions(+), 61 deletions(-) create mode 100644 clang/test/SemaTemplate/constraints.cpp diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 202f0f2..5adb66d 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -6394,15 +6394,10 @@ public: /// A diagnostic is emitted if it is not, false is returned, and /// PossibleNonPrimary will be set to true if the failure might be due to a /// non-primary expression being used as an atomic constraint. - bool CheckConstraintExpression(Expr *CE, Token NextToken = Token(), + bool CheckConstraintExpression(const Expr *CE, Token NextToken = Token(), bool *PossibleNonPrimary = nullptr, bool IsTrailingRequiresClause = false); - /// Check whether the given type-dependent expression will be the name of a - /// function or another callable function-like entity (e.g. a function - // template or overload set) for any substitution. - bool IsDependentFunctionNameExpr(Expr *E); - private: /// Caches pairs of template-like decls whose associated constraints were /// checked for subsumption and whether or not the first's constraints did in diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp index 290e4cb..ddd95fa 100755 --- a/clang/lib/Sema/SemaConcept.cpp +++ b/clang/lib/Sema/SemaConcept.cpp @@ -28,21 +28,47 @@ using namespace clang; using namespace sema; -bool -Sema::CheckConstraintExpression(Expr *ConstraintExpression, Token NextToken, - bool *PossibleNonPrimary, - bool IsTrailingRequiresClause) { +namespace { +class LogicalBinOp { + OverloadedOperatorKind Op = OO_None; + const Expr *LHS = nullptr; + const Expr *RHS = nullptr; + +public: + LogicalBinOp(const Expr *E) { + if (auto *BO = dyn_cast(E)) { + Op = BinaryOperator::getOverloadedOperator(BO->getOpcode()); + LHS = BO->getLHS(); + RHS = BO->getRHS(); + } else if (auto *OO = dyn_cast(E)) { + Op = OO->getOperator(); + LHS = OO->getArg(0); + RHS = OO->getArg(1); + } + } + + bool isAnd() const { return Op == OO_AmpAmp; } + bool isOr() const { return Op == OO_PipePipe; } + explicit operator bool() const { return isAnd() || isOr(); } + + const Expr *getLHS() const { return LHS; } + const Expr *getRHS() const { return RHS; } +}; +} + +bool Sema::CheckConstraintExpression(const Expr *ConstraintExpression, + Token NextToken, bool *PossibleNonPrimary, + bool IsTrailingRequiresClause) { // C++2a [temp.constr.atomic]p1 // ..E shall be a constant expression of type bool. ConstraintExpression = ConstraintExpression->IgnoreParenImpCasts(); - if (auto *BinOp = dyn_cast(ConstraintExpression)) { - if (BinOp->getOpcode() == BO_LAnd || BinOp->getOpcode() == BO_LOr) - return CheckConstraintExpression(BinOp->getLHS(), NextToken, - PossibleNonPrimary) && - CheckConstraintExpression(BinOp->getRHS(), NextToken, - PossibleNonPrimary); + if (LogicalBinOp BO = ConstraintExpression) { + return CheckConstraintExpression(BO.getLHS(), NextToken, + PossibleNonPrimary) && + CheckConstraintExpression(BO.getRHS(), NextToken, + PossibleNonPrimary); } else if (auto *C = dyn_cast(ConstraintExpression)) return CheckConstraintExpression(C->getSubExpr(), NextToken, PossibleNonPrimary); @@ -60,7 +86,7 @@ Sema::CheckConstraintExpression(Expr *ConstraintExpression, Token NextToken, (NextToken.is(tok::l_paren) && (IsTrailingRequiresClause || (Type->isDependentType() && - IsDependentFunctionNameExpr(ConstraintExpression)) || + isa(ConstraintExpression)) || Type->isFunctionType() || Type->isSpecificBuiltinType(BuiltinType::Overload))) || // We have the following case: @@ -99,39 +125,37 @@ calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr, AtomicEvaluator &&Evaluator) { ConstraintExpr = ConstraintExpr->IgnoreParenImpCasts(); - if (auto *BO = dyn_cast(ConstraintExpr)) { - if (BO->getOpcode() == BO_LAnd || BO->getOpcode() == BO_LOr) { - if (calculateConstraintSatisfaction(S, BO->getLHS(), Satisfaction, - Evaluator)) - return true; + if (LogicalBinOp BO = ConstraintExpr) { + if (calculateConstraintSatisfaction(S, BO.getLHS(), Satisfaction, + Evaluator)) + return true; - bool IsLHSSatisfied = Satisfaction.IsSatisfied; + bool IsLHSSatisfied = Satisfaction.IsSatisfied; - if (BO->getOpcode() == BO_LOr && IsLHSSatisfied) - // [temp.constr.op] p3 - // A disjunction is a constraint taking two operands. To determine if - // a disjunction is satisfied, the satisfaction of the first operand - // is checked. If that is satisfied, the disjunction is satisfied. - // Otherwise, the disjunction is satisfied if and only if the second - // operand is satisfied. - return false; + if (BO.isOr() && IsLHSSatisfied) + // [temp.constr.op] p3 + // A disjunction is a constraint taking two operands. To determine if + // a disjunction is satisfied, the satisfaction of the first operand + // is checked. If that is satisfied, the disjunction is satisfied. + // Otherwise, the disjunction is satisfied if and only if the second + // operand is satisfied. + return false; - if (BO->getOpcode() == BO_LAnd && !IsLHSSatisfied) - // [temp.constr.op] p2 - // A conjunction is a constraint taking two operands. To determine if - // a conjunction is satisfied, the satisfaction of the first operand - // is checked. If that is not satisfied, the conjunction is not - // satisfied. Otherwise, the conjunction is satisfied if and only if - // the second operand is satisfied. - return false; + if (BO.isAnd() && !IsLHSSatisfied) + // [temp.constr.op] p2 + // A conjunction is a constraint taking two operands. To determine if + // a conjunction is satisfied, the satisfaction of the first operand + // is checked. If that is not satisfied, the conjunction is not + // satisfied. Otherwise, the conjunction is satisfied if and only if + // the second operand is satisfied. + return false; - return calculateConstraintSatisfaction(S, BO->getRHS(), Satisfaction, - std::forward(Evaluator)); - } - } - else if (auto *C = dyn_cast(ConstraintExpr)) + return calculateConstraintSatisfaction( + S, BO.getRHS(), Satisfaction, std::forward(Evaluator)); + } else if (auto *C = dyn_cast(ConstraintExpr)) { return calculateConstraintSatisfaction(S, C->getSubExpr(), Satisfaction, std::forward(Evaluator)); + } // An atomic constraint expression ExprResult SubstitutedAtomicExpr = Evaluator(ConstraintExpr); @@ -725,19 +749,16 @@ NormalizedConstraint::fromConstraintExpr(Sema &S, NamedDecl *D, const Expr *E) { // - The normal form of an expression (E) is the normal form of E. // [...] E = E->IgnoreParenImpCasts(); - if (auto *BO = dyn_cast(E)) { - if (BO->getOpcode() == BO_LAnd || BO->getOpcode() == BO_LOr) { - auto LHS = fromConstraintExpr(S, D, BO->getLHS()); - if (!LHS) - return None; - auto RHS = fromConstraintExpr(S, D, BO->getRHS()); - if (!RHS) - return None; + if (LogicalBinOp BO = E) { + auto LHS = fromConstraintExpr(S, D, BO.getLHS()); + if (!LHS) + return None; + auto RHS = fromConstraintExpr(S, D, BO.getRHS()); + if (!RHS) + return None; - return NormalizedConstraint( - S.Context, std::move(*LHS), std::move(*RHS), - BO->getOpcode() == BO_LAnd ? CCK_Conjunction : CCK_Disjunction); - } + return NormalizedConstraint(S.Context, std::move(*LHS), std::move(*RHS), + BO.isAnd() ? CCK_Conjunction : CCK_Disjunction); } else if (auto *CSE = dyn_cast(E)) { const NormalizedConstraint *SubNF; { diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 6f25f71..e8be40c3 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -18945,11 +18945,6 @@ ExprResult Sema::ActOnObjCAvailabilityCheckExpr( ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy); } -bool Sema::IsDependentFunctionNameExpr(Expr *E) { - assert(E->isTypeDependent()); - return isa(E); -} - ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End, ArrayRef SubExprs, QualType T) { // FIXME: enable it for C++, RecoveryExpr is type-dependent to suppress diff --git a/clang/test/SemaTemplate/constraints.cpp b/clang/test/SemaTemplate/constraints.cpp new file mode 100644 index 0000000..0bc4727 --- /dev/null +++ b/clang/test/SemaTemplate/constraints.cpp @@ -0,0 +1,26 @@ +// RUN: %clang_cc1 -std=c++20 -verify %s +// RUN: %clang_cc1 -std=c++20 -verify %s -DDEPENDENT_OR + +#ifdef DEPENDENT_OR +// This causes the || below to be a CXXOperatorCallExpr not a BinaryOperator. +struct A {}; bool operator||(A, A); +#endif + +namespace PR45589 { + template struct X { static constexpr bool value = T::value; }; // expected-error {{cannot be used prior to '::'}} + struct False { static constexpr bool value = false; }; + struct True { static constexpr bool value = true; }; + + template concept C = true; + + template constexpr int test = 0; + template requires C constexpr int test = 1; + template requires (B && C) || (X::value && C) constexpr int test = 2; // expected-error {{non-constant expression}} expected-note {{subexpression}} expected-note {{instantiation of}} expected-note {{while substituting}} + static_assert(test == 2); + static_assert(test == 2); + static_assert(test == 2); // satisfaction of second term of || not considered + static_assert(test == 1); + static_assert(test == 2); // constraints are partially ordered + // FIXME: These diagnostics are excessive. + static_assert(test == 1); // expected-note 2{{while}} expected-note 2{{during}} +} -- 2.7.4