[PATCH] D155858: Add a concept AST node.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK, so I learned a bit about initializeLocal/trivial TypeSourceInfo. Mostly things I didn't want to know :-)
TL;DR I think the current version of the patch is right.

The crash is caused by:

1. `CheckTemplateArgument` calls `SubstType` without passing a `TypeLoc`, which synthesizes a trivial `TypeSourceInfo` to instantiate
2. the trivial `TypeSourceInfo` never has a `ConceptReference`
3. instantiating an `AutoTypeLoc` where the type is constrained but there's no `ConceptReference` is not supported

We can choose to address any of these.

Fixing #1 is tempting: the SubstType overload with no TypeLoc is deprecated and calling it ~always loses information. Cleaning up this call is an improvement, but there are ~15 calls to this overload, and probably more places that use trivial TypeSourceInfo.
So this is nice to have, but I think leaves us with an AST that will still crash in other cases. Consumers will not expect an AutoTypeLoc where the type is constrained but the loc isn't, and it's hard to know how to handle this case.

Fixing #2 is what this patch currently does. It also seems to be the pattern used by other types (their initializeLocal methods conditionally initialize structure based on the underlying type). The only possible downside is because we store the ConceptReference subobject by pointer and support replacement but not mutation, if the caller calls initializeLocal and then fills in with real data we'll waste the original ConceptReference object. Still, I don't think this is how non-trivial TSIs are generally created, and I think this is the way to go.

Fixing #3 is the previous version of the patch, again it leaves us with a bug-prone AST and some extra complexity.



================
Comment at: clang/include/clang/AST/ASTConcept.h:183
+    if (auto QualifierLoc = getNestedNameSpecifierLoc())
+      return QualifierLoc.getBeginLoc();
+    return getConceptNameInfo().getBeginLoc();
----------------
if the qualifier is null the template KW must also be null
so this is correct, you may or may not want a comment for that! up to you


================
Comment at: clang/include/clang/AST/ASTConcept.h:188
+  SourceLocation getEndLoc() const LLVM_READONLY {
+    return getTemplateArgsAsWritten()->getRAngleLoc().isValid()
+               ? getTemplateArgsAsWritten()->getRAngleLoc()
----------------
getTemplateArgsAsWritten() is nullable, right?


================
Comment at: clang/include/clang/AST/ASTConcept.h:215
+
+  void print(llvm::raw_ostream &OS, PrintingPolicy Policy) const;
 };
----------------
nit: printingpolicy by const ref, I think
(we're not totally consistent, but I think ref is more common)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155858