[PATCH] D155858: Add a concept AST node.

[Jens Massberg via Phabricator via cfe-commits]

massberg marked an inline comment as done.
massberg added inline comments.


================
Comment at: clang/include/clang/AST/ASTConcept.h:118
 /// template arguments.
-class ConceptReference {
+class ConceptLoc {
 protected:
----------------
cor3ntin wrote:
> hokein wrote:
> > I'm not sure the  `ConceptLoc` is a reasonable name for this structure.
> > 1) In general, classes named `*Loc` in clang AST are tiny, and they are passed by value, so seeing the usage `ConceptLoc *` in the code is a bit wired and inconsistent.
> > 2) Unlike other `TemplateArgumentLoc`, `NestedNameSpecifierLoc`, `TypeLoc` (where they all have TemplateArgument/TemplateArgumentLoc split), and we don't have a corresponding split for `ConceptLoc`.
> > 
> > I think 2) is probably fine, but 1) is a concern, we plan to make it a real AST node, so we will likely add it to the [DynTypedNode](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/ASTTypeTraits.h#L508), adding a big class there will increase the `DynTypedNode` a lot (from 40 bytes => 80 bytes).  
> > 
> > One idea is to make this class `ConceptLocInfo`, and create a new wrapper class `ConceptLoc` wrapper which contains a `ConceptLocInfo* pointer`, and a `ConceptDecl* concept`.
> > 
> > 
> Agreed, the name change seems more confusing than anything and not really consistent with preexisting names.
> It is still unclear to me what is wrong with the current name (and all these renamings obfuscate what the patch is actually trying to do)
I'm sorry if the renaming led to confusions. I have renamed it back to `ConceptReference` for now. I'm happy for any proposals for a good name (or to keep the old name).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155858