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