[PATCH] D41217: [Concepts] Concept Specialization Expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/ExprCXX.h:4420
+  /// \brief The concept named.
+  ConceptDecl *NamedConcept;
+
----------------
saar.raz wrote:
> rsmith wrote:
> > You should also track the `FoundDecl` and the optional `NestedNameSpecifierLoc` (just like a `DeclRefExpr` would). Clang-based tools (particularly refactoring tools) need those. There's also an optional `template` keyword, but it can never matter in practice because the nested name specifier can never be dependent, so it's probably not the most high-priority thing to track.
> Isn't the `FoundDecl` just the `NamedConcept`?
Not in general, no. The `FoundDecl` could be a `UsingShadowDecl`.


================
Comment at: lib/AST/ExprCXX.cpp:1472
+  for (const TemplateArgumentLoc& LocInfo : ArgsAsWritten->arguments()) {
+    if (LocInfo.getArgument().isInstantiationDependent()) {
+      IsDependent = true;
----------------
This is incorrect. Whether the expression is dependent is not the same thing as whether it's instantiation-depnedent. A concept specialization should be considered instantiation-dependent if it has any instantiation-dependent template arguments, and should be considered value-dependent if it has any dependent template arguments.

For example, `Concept<sizeof(sizeof(T))>` has an instantiation-dependent template argument, but no dependent template arguments, so should be instantiation-dependent (the validity of the construct can depend on the type `T`) but not value-dependent (the value of the construct is always `Concept<sizeof(size_t)>`, which doesn't depend on `T`).


================
Comment at: lib/AST/StmtPrinter.cpp:2553
+void StmtPrinter::VisitConceptSpecializationExpr(ConceptSpecializationExpr *E) {
+  OS << E->getNamedConcept()->getName();
+  printTemplateArgumentList(OS, E->getTemplateArgumentListInfo()->arguments(),
----------------
saar.raz wrote:
> rsmith wrote:
> > You should print out the nested name specifier here. (And ideally also the `template` keyword, if specified...). And you should print the name of the found declaration, not the name of the concept (although they can't differ currently).
> Are there possible upcoming changes that'll make them differ?
p0945r0 would allow them to differ.


================
Comment at: lib/Sema/SemaConcept.cpp:105-112
+  if (!E.get()->EvaluateAsBooleanCondition(IsSatisfied, Context)) {
+      // C++2a [temp.constr.atomic]p1
+      //   ...E shall be a constant expression of type bool.
+    Diag(E.get()->getLocStart(),
+         diag::err_non_constant_constraint_expression)
+        << E.get()->getSourceRange();
+    return true;
----------------
Please collect the notes from `EvalutaeAsBooleanCondition` and emit them here, so the user knows /why/ the expression was non-constant.


================
Comment at: lib/Sema/SemaTemplate.cpp:3907-3911
+  for (TemplateArgument &Arg : Converted)
+    if (Arg.isInstantiationDependent()) {
+      IsDependent = true;
+      break;
+    }
----------------
Add braces around this `for`, because its body contains multiple statements. (Generally, if an inner construct has braces we want outer constructs to have them too.)


================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:679-681
+      Diags.Report(Active->PointOfInstantiation,
+                   diag::note_constraint_substitution_here)
+          << Active->InstantiationRange;
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D41217