mizvekov added inline comments.
================ Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList *getReplacedTemplateParameterList(Decl *D); + ---------------- 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. 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
