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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
