[PATCH] D155858: Add a concept AST node.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So I did a quick run through, I think this is a fine idea and an improvement.  However, @sammccall has given some very good feedback that I'd like to have him do the final approval.



================
Comment at: clang/include/clang/AST/ASTConcept.h:213
   Expr *ImmediatelyDeclaredConstraint = nullptr;
+  ConceptReference *CR;
 
----------------
Preference to name this something like `ConceptRef` instead of `CR` here.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:49
 protected:
+  ConceptReference *CR;
+
----------------
Same comment here.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:149
     // there may not be a template argument list.
-    return ArgsAsWritten->RAngleLoc.isValid() ? ArgsAsWritten->RAngleLoc
-                                              : ConceptName.getEndLoc();
+    return CR->hasExplicitTemplateArgs() &&
+                   CR->getTemplateArgsAsWritten()->getRAngleLoc().isValid()
----------------
What did you find that resulted in this change?  What does `hasExplicitTemplateArgs` 'mean' when we have empty arguments, so something like:

    template<UnaryConcept<> C>
    void foo(const C&){}

Basically, the end loc should ALWAYS be the right-angle if it exists, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155858/new/

https://reviews.llvm.org/D155858