[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via lldb-commits]

davrec added inline comments.


================
Comment at: clang/include/clang/AST/DeclTemplate.h:3427
 
+TemplateParameterList *getReplacedTemplateParameterList(Decl *D);
+
----------------
mizvekov wrote:
> mizvekov wrote:
> > davrec wrote:
> > > davrec wrote:
> > > > I don't object with making this public, and I can see the argument for making this its own function rather than a Decl method all things being equal, but given that we already have `Decl::getDescribedTemplateParams`, I think that 
> > > > # this should probably also go in Decl,
> > > > # a good comment is needed explaining the difference between the two (e.g. that the `D` is the template/template-like decl itself, not a templated decl as required by `getDescribedTemplateParams`, or whatever), and what happens when it is called on a non-template decl, and
> > > > # it probably should be named just `getTemplateParams` or something that suggests its difference with `getDescribedTemplateParams`.
> > > > 
> > > On second thought this should definitely be a Decl method called `getTemplateParameters()`, since all it does is dispatches to the derived class's implementation of that.
> > I think this function shouldn't be public in that sense, it should only be used for implementation of retrieving parameter for Subst* nodes.
> > 
> > I don't think this should try to handle any other kinds of decls which can't appear as the AssociatedDecl in a Subst node.
> > 
> > There will be Subst specific changes here in the future, but which depend on some other enablers:
> > * We need to make Subst nodes for variable templates store the SpecializationDecl instead of the TemplateDecl, but this requires changing the order between creating the specialization and performing the substitution. I will do that in a separate patch.
> > * We will stop creating Subst* nodes for things we already performed substitution with sugar. And so, we won't need to handle alias templates, conceptdecls, etc. Subst nodes will only be used for things which we track specializations for, and that need resugaring.
> > 
> > It's only made 'public' because now we are using it across far away places like `Type.cpp` and `ExprCXX.cpp` and `TemplateName.cpp`.
> > 
> > I didn't think this needs to go as a Decl member, because of limited scope and because it only ever needs to access public members.
> > I don't think this justifies either a separate header to be included only where it's needed.
> > 
> > One option is to further make the name more specific, by adding `Internal` to the name for example.
> > Any other ideas?
> I ended up simply documenting that this is an internal function, for now.
That works for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858