[PATCH] D155858: Add a concept AST node.

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/include/clang/AST/ASTConcept.h:118
 /// template arguments.
-class ConceptReference {
+class ConceptLoc {
 protected:
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155858