[PATCH] D123127: [AST] Add a new TemplateName for templates found via a using declaration.

[Sam McCall via Phabricator via cfe-commits]

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