[PATCH] D41217: [Concepts] Concept Specialization Expressions
Saar Raz via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 13 11:23:18 PDT 2019
saar.raz marked 14 inline comments as done and 2 inline comments as done.
saar.raz added inline comments.
================
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;
----------------
rsmith wrote:
> If an `operator&&` or `operator||` function is declared, this could be a `CXXOperatorCallExpr` instead. You will need to recurse into those constructs too.
Atomic constraints will have bool types anyway, can this really happen?
================
Comment at: lib/Sema/SemaConcept.cpp:72-74
+ else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr))
+ return CalculateConstraintSatisfaction(NamedConcept, MLTAL, C->getSubExpr(),
+ IsSatisfied);
----------------
rsmith wrote:
> 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.
Since this part will be changed in later patches, I'd like to delay this fix to a later patch for now.
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:679-681
+ Diags.Report(Active->PointOfInstantiation,
+ diag::note_constraint_substitution_here)
+ << Active->InstantiationRange;
----------------
saar.raz wrote:
> saar.raz wrote:
> > rsmith wrote:
> > > Is this note ever useful? It will presumably always point into the same concept definition that the prior diagnostic also pointed at, and doesn't seem to add anything in the testcases.
> > >
> > > Maybe we could keep the CodeSynthesisContext around as a marker that we've entered a SFINAE context, but not have any corresponding diagnostic. (The note produced for the enclosing `ConstraintsCheck` context covers that.) Or we could remove this and store a flag on the `ConstraintsCheck` to indicate whether we're in a SFINAEable portion of it.
> > If the concept definition is multiline/contains macros, this would point at the exact place where the problematic constraint occured, we should probably add tests for this case though.
> > Maybe we can omit the diagnostic when the concept and the constraint are on the same line or something?
> >
> Correction - I just now realized you were referring to the 'in instantiation of template class' note and not the 'while checking the satisfaction...' note.
> In this case - the only case I can think of where the problematic instantiation and the constraint expression would be in very different places is indeed multiline constraint expressions or macros in a constraint expression (but you know better than I do - can you think of any other cases? or maybe there are other non-SFINAE errors that can result from substitution and would not produce a note?). Maybe these cases are remote enough to warrant removing this diagnostic or as I said earlier omit them in cases where they are unhelpful.
In any case, I'm in favor of pushing this to a separate patch (after we have diagnostics for requires exprs, which would suffer from a similar problem)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41217/new/
https://reviews.llvm.org/D41217
More information about the cfe-commits
mailing list