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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
