[PATCH] D155858: Add a concept AST node.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LGTM, just nits!

The SourceRanges are actually correct :-)



================
Comment at: clang/include/clang/AST/ASTConcept.h:124
 class ConceptReference {
 protected:
   // \brief The optional nested name specifier used when naming the concept.
----------------
remove `protected` (and `private:` below), we don't inherit from this


================
Comment at: clang/include/clang/AST/ExprConcepts.h:46
 public:
   using SubstitutionDiagnostic = std::pair<SourceLocation, std::string>;
 
----------------
while here: this is unused, remove?


================
Comment at: clang/include/clang/AST/ExprConcepts.h:48
 
 protected:
+  ConceptReference *ConceptRef;
----------------
while here: this class is final, so these can be private instead of protected


================
Comment at: clang/lib/AST/TypeLoc.cpp:625
 
-DeclarationNameInfo AutoTypeLoc::getConceptNameInfo() const {
-  return DeclarationNameInfo(getNamedConcept()->getDeclName(),
-                             getLocalData()->ConceptNameLoc);
+static ConceptReference *createConceptReference(ASTContext &Context,
+                                                SourceLocation Loc,
----------------
nit: createTrivialConceptReference?
hinting at the relationship to trivial TypeSourceInfo

maybe a comment too?
```
// Builds a ConceptReference where all locations point at the same token,
// for use in trivial TypeSourceInfo for constrained AutoType
```


================
Comment at: clang/unittests/AST/SourceLocationTest.cpp:1025
+)cpp";
+  // FIXME: expected range should be (2, 1, 2, 3)
+  Verifier.expectRange(2, 1, 2, 1);
----------------
why that range?
SourceRanges are generally "closed token ranges", so the endpoint is the *beginning* of the last token *in* the range.
`CCC` is (2, 1, 2, 1)


================
Comment at: clang/unittests/AST/SourceLocationTest.cpp:1073
+)cpp";
+  // FIXME: expected range should be (2, 11, 2, 13)
+  Verifier.expectRange(2, 11, 2, 11);
----------------
as above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155858