[PATCH] D41217: [Concepts] Concept Specialization Expressions

[Saar Raz via Phabricator via cfe-commits]

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