sammccall added inline comments.
================ Comment at: clang/include/clang/AST/TemplateName.h:404 + + TemplateName UnderlyingTN; + UsingShadowDecl *USD = nullptr; ---------------- (Per offline discussion) The UnderlyingTN here seems like it would refer to the TemplateDecl pointed to by the UsingShadowDecl, with sugar from the RHS of the UsingDecl. The only sugar possible is a namespace [alias] qualifier, so the only TemplateNames possible are Qualified and Ordinary. And the code isn't actually creating Qualified names here when it would be applicable. So we should either: - change this to be a TemplateDecl* instead - start creating the Qualified names where appropriate Arguments in favor of having the sugar qualified names: - it's a more complete AST - it's more consistent with sugar types Arguments against the sugar qualified names - performance: it avoids adding a second level of indirection, extra storage etc - it avoids confusing callers into handling *all* TemplateName cases here - the sugar is still available: USD->getUsingDecl()->getQualifier() I think we should not provide sugared names here, and change the type to TemplateDecl* ================ Comment at: clang/include/clang/AST/TemplateName.h:404 + + TemplateName UnderlyingTN; + UsingShadowDecl *USD = nullptr; ---------------- sammccall wrote: > (Per offline discussion) > > The UnderlyingTN here seems like it would refer to the TemplateDecl pointed > to by the UsingShadowDecl, with sugar from the RHS of the UsingDecl. > > The only sugar possible is a namespace [alias] qualifier, so the only > TemplateNames possible are Qualified and Ordinary. And the code isn't > actually creating Qualified names here when it would be applicable. > > So we should either: > - change this to be a TemplateDecl* instead > - start creating the Qualified names where appropriate > > Arguments in favor of having the sugar qualified names: > - it's a more complete AST > - it's more consistent with sugar types > > Arguments against the sugar qualified names > - performance: it avoids adding a second level of indirection, extra storage > etc > - it avoids confusing callers into handling *all* TemplateName cases here > - the sugar is still available: USD->getUsingDecl()->getQualifier() > > I think we should not provide sugared names here, and change the type to > TemplateDecl* If we change the type to TemplateDecl, do we actually need to store it? Or can we just `cast<TemplateDecl>(USD->getTargetDecl())`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123127/new/ https://reviews.llvm.org/D123127 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
