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