davrec added inline comments.
================
Comment at: clang/include/clang/AST/ASTContext.h:1618
+ QualType getSubstTemplateTypeParmType(QualType Replacement,
+ Decl *ReplacedDecl,
+ unsigned Index) const;
----------------
mizvekov wrote:
> davrec wrote:
> > ReplacedDecl -> ReplacedParamParent (see below)
> We need a more apt name.
>
> How about `ReplacedTemplateLikeEntityDecl`?
Problem is if it's a `TemplateTypeParmDecl` its not even a template like
entity. That's why I lean towards "AssociatedDecl" or something like that,
that is just so vague users are forced to look up the documentation to
understand it.
================
Comment at: clang/include/clang/AST/Type.h:4998
+ /// Gets the templated entity that was substituted.
+ Decl *getReplacedDecl() const { return ReplacedDecl; }
+
----------------
mizvekov wrote:
> davrec wrote:
> > To reiterate an earlier comment which may have been overlooked: I think
> > this needs a clearer name and documentation (so long as I do not
> > misunderstand what kinds of decls it may be - see other inline comment).
> >
> > Given that we have `getReplacedParameter()`, I think something like
> > `getReplacedParamParent()` makes the most sense.
> >
> > And the documentation should suggest the relationship between
> > `getReplacedParameter` and this (i.e. `getReplacedParamParent()` +
> > `getIndex()` -> `getReplacedParameter()`).
> >
> > Plus, add back in the original documentation for `getReplacedParameter()`,
> > and a brief line of documentation for `getIndex()` (and do same for the
> > Pack version below too).
> >
> Yeah like I said, we can't suggest a relationship very much, I don't think we
> should try to explain how to obtain the parameter from the main entity, that
> would be exposing too much guts.
>
> We just offer a method to get it, and hope for the best.
>
> The name for this method, whatever we pick, will probably be a bit vague
> sounding, because in Clang a template is a very vague concept (in the
> philosophical sense and in the C++20 sense).
Let's say we call it `getAssociatedDecl()`. Whatever the name I strongly think
we need some good documentation explaining what these return, if they are going
to be public methods. How about something like this:
```
/// Gets the template parameter declaration that was substituted for.
const TemplateTypeParmDecl *getReplacedParameter() const;
/// If the replaced TemplateTypeParmDecl belongs to a parent template/
/// template-like declaration, returns that parent.
/// Otherwise, returns the replaced TTPD.
Decl *getAssociatedDecl() const { return AssociatedDecl; }
/// If the replaced parameter belongs to a parent template/template-like
/// declaration, returns the index of the parameter in that parent.
/// Otherwise, returns zero.
unsigned getIndex() const { return SubstTemplateTypeParmTypeBits.Index; }
```
================
Comment at: clang/lib/AST/Type.cpp:3709
+ unsigned Index) {
+ if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(D))
+ return TTP;
----------------
mizvekov wrote:
> davrec wrote:
> > Will this cast ever succeed? I.e. are there still places
> > `Context.getSubstTemplateTypeParmType(...)` is called with a TTPD as the
> > `ReplacedDecl`? That would make `getReplacedDecl()` much more
> > complicated/less understandable - can we change any such places to always
> > pass the parent template/templated entity as the ReplacedDecl, for
> > consistency (and so that we can rename ReplacedDecl to ReplacedParamParent)?
> Yeah, there is this one place related to concepts where we invent a
> substitution and there is actually no associated declaration at all, besides
> just the invented template parameter, so in this case the TTP is the parent
> and parameter.
>
> So that is why I picked such a vaguely named acessor... because there is no
> order here, anarchy reigns.
I see. So close. I agree we need to go vague then. I think ReplacedDecl
isn't quite vague enough though; a user could easily conflate
getReplacedParameter and getReplacedDecl. I think you hit on a better term in
your wording though: "AssociatedDecl". So long as it is documented I think
that could work.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131858/new/
https://reviews.llvm.org/D131858
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
