[PATCH] D41217: [Concepts] Concept Specialization Expressions
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 16 11:07:36 PDT 2019
rsmith marked an inline comment as done.
rsmith added inline comments.
================
Comment at: include/clang/Sema/Sema.h:5904
+ Expr *ConstraintExpr,
+ bool &IsSatisfied);
+
----------------
(Nit: please align the second and subsequent parameters here with the first one.)
================
Comment at: include/clang/Sema/Sema.h:7487-7488
+ // We are checking the constraints associated with a constrained entity or
+ // the constraint expression of a concept.
+ ConstraintsCheck,
----------------
It'd be useful to note that this context covers the check that atomic constraints have type `bool` and can be constant-evaluated. (It's not obvious how we'd hit diagnostics in this case, since most substitution failures result in an unsatisfied constraint.)
================
Comment at: lib/AST/ExprCXX.cpp:1703-1704
+ bool IsDependent = false;
+ bool IsInstantiationDependent = false;
+ bool ContainsUnexpandedParameterPack = false;
+ for (const TemplateArgumentLoc& LocInfo : ArgsAsWritten->arguments()) {
----------------
It'd be nice to assert that the nested name specifier is neither instantiation-dependent nor contains an unexpanded parameter pack. (That's true today because the NNS of a concept specialization expression can only refer to a namespace.) If the NNS could be instantiation-dependent or contain an unexpanded pack, we should inherit those properties here.
================
Comment at: lib/AST/ExprCXX.cpp:1708-1709
+ IsDependent = true;
+ if (ContainsUnexpandedParameterPack && IsInstantiationDependent)
+ break;
+ }
----------------
This code would be a little clearer if you moved an `if (IsDependent && ContainsUnexpandedParameterPack && IsInstantiationDependent)` check to the end of the loop. (I doubt the change will ever make any performance difference.)
================
Comment at: lib/AST/StmtProfile.cpp:1303-1305
+ VisitDecl(S->getNamedConcept());
+ VisitTemplateArguments(S->getTemplateArgsAsWritten()->getTemplateArgs(),
+ S->getTemplateArgsAsWritten()->NumTemplateArgs);
----------------
It seems a little inconsistent to use the named concept rather than the found decl (that is, resolved rather than as-written), but to use the as-written template arguments rather than the resolved and converted arguments. (I don't think it matters either way, since the profiler is in principle allowed to use either the as-written or resolved form, but generally it should prefer to use the as-written form.)
================
Comment at: lib/Sema/SemaConcept.cpp:34
+ Diag(ConstraintExpression->getExprLoc(),
+ diag::err_non_bool_atomic_constraint)
+ << ConstraintExpression << ConstraintExpression->getType();
----------------
saar.raz wrote:
> saar.raz wrote:
> > rsmith wrote:
> > > What justifies rejecting this prior to any use of the concept that would result in a satisfaction check?
> > >
> > > (I think checking this is a good thing; what I'm wondering is whether we need to file a core issue to get the wording updated to allow us to reject such bogus concepts even if they're never used.)
> > I guess this is already justified, if awkwardly (NDR) by [temp.res]p8.2
> Correction - it says that IFNDR occurs when no substitution would result in a valid expression, so maybe this is well formed after all.
> In this case it is a valid expression but not a valid constraint expression, maybe that's the missing word here?
OK, I'll take this to the core reflector to make sure the wording is clear this is ill-formed, no diagnostic required.
================
Comment at: lib/Sema/SemaConcept.cpp:37-41
+ } else if (ImplicitCastExpr *E =
+ dyn_cast<ImplicitCastExpr>(ConstraintExpression)) {
+ // This will catch expressions such as '2 && true'
+ return CheckConstraintExpression(E->getSubExpr());
+ }
----------------
rsmith wrote:
> Call `IgnoreParenImpCasts` before checking the type of an atomic constraint, rather than adding an extra recursive step here.
I think we're missing this, and we won't reject:
```
template<typename T> concept bool X = true && (0 && 0);
```
because we treat `(0 && 0)` as an atomic constraint rather than treating the two `0` subexpressions as atomic constraints here.
(Maybe add `ConstraintExpression = ConstraintExpression->IgnoreParenImpCasts();` or something like that to the start of the function?)
================
Comment at: lib/Sema/SemaConcept.cpp:29
+ return CheckConstraintExpression(BinOp->getLHS()) &&
+ CheckConstraintExpression(BinOp->getRHS());
+
----------------
Nit: indent the second line so that the two `CheckConstraintExpression` calls line up.
================
Comment at: lib/Sema/SemaConcept.cpp:51-68
+ if (auto *BO = dyn_cast<BinaryOperator>(ConstraintExpr)) {
+ if (BO->getOpcode() == BO_LAnd) {
+ if (CalculateConstraintSatisfaction(NamedConcept, MLTAL, BO->getLHS(),
+ IsSatisfied))
+ return true;
+ if (!IsSatisfied)
+ return false;
----------------
If an `operator&&` or `operator||` function is declared, this could be a `CXXOperatorCallExpr` instead. You will need to recurse into those constructs too.
================
Comment at: lib/Sema/SemaConcept.cpp:69-71
+ } else if (auto *PO = dyn_cast<ParenExpr>(ConstraintExpr))
+ return CalculateConstraintSatisfaction(NamedConcept, MLTAL,
+ PO->getSubExpr(), IsSatisfied);
----------------
Consider using `IgnoreParenImpCasts` here rather than manually handling these cases. (We should be consistent in the mechanism by which we traverse constraint expressions.)
This will do different things particularly for certain language extensions, such as `_Generic`, `__builtin_choose_expr`, and `__extension` (which are treated as being parens-like).
================
Comment at: lib/Sema/SemaConcept.cpp:72-74
+ else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr))
+ return CalculateConstraintSatisfaction(NamedConcept, MLTAL, C->getSubExpr(),
+ IsSatisfied);
----------------
This case is not handled by `CheckConstraintExpression`; we should be consistent in whether we step over these or not.
Perhaps it would make sense to factor out a mechanism to take an expression and classify it as atomic constraint (with the corresponding expression), or conjunction (with a pair of subexpressions), or disjunction (with a pair of subexpressions), and use that everywhere we traverse a constraint expression.
================
Comment at: lib/Sema/SemaTemplate.cpp:4256-4264
+ bool IsSatisfied = true;
+ bool IsInstantiationDependent = false;
+ for (TemplateArgument &Arg : Converted) {
+ if (Arg.isInstantiationDependent()) {
+ IsInstantiationDependent = true;
+ break;
+ }
----------------
`isSatisfied` only looks at whether the expression is value-dependent; if it's instantiation-dependent but not value-dependent, then you won't have computed whether it's satisfied but will still permit access to that value. We should be checking whether the arguments are dependent here, not whether they're value-dependent. (And since you need to compute that here anyway, I think it'd make more sense to pass in an "is value-dependent" flag to `ConstraintSpecializationExpr::Create`.)
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:522
<< Active->InstantiationRange;
+ } else {
}
----------------
Please delete this empty `else` block (I assume this is left behind from prior changes).
================
Comment at: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:8-11
+template<typename T> concept C2 = sizeof(T) == 4;
+static_assert(C2<int>);
+static_assert(!C2<long long int>);
+static_assert(C2<char[4]>);
----------------
These tests at least theoretically depend on the target; please add a `-triple x86_64-linux-gnu` or whatever to this file so that this test doesn't fail on targets where `sizeof(int)` is not 4. (I'm not sure we have any such targets right now, but it's not guaranteed.)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41217/new/
https://reviews.llvm.org/D41217
More information about the cfe-commits
mailing list