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