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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/AST/TemplateName.cpp:246
                  TemplateNameDependenceScope::DependentInstantiation)
       Template->printQualifiedName(OS, Policy);
     else
----------------
hokein wrote:
> sammccall wrote:
> > this is going to print the wrong qualifier: qualifier of the underlying rather than of the usingdecl
> I think this is a good argument -- after the code `namespace ns { using std::vector;  }`, what's the fully-qualified name of the `vector` UsingTemplateName within ns?
> 
> 1) `ns::vector` --  the qualified name of using-decl.
> 2) or `std::vector`- the qualified name of the underlying template decl.
> 
> after a second thought, I'm leaning towards 2). AFAIK, the major place to print fully-qualified name is `TemplateArgument::print` (for template template parameters). 2) is providing more useful information for users to locate the actual template class. Imaging a case where we have a `using long::long::long::abc` in a global namespace, printing `abc` seems less useful. 
> 
> This is also consistent with `UsingType`.
OK, that makes sense, but deserves a serious comment - it *looks* like a bug.

(I'd forgotten about the UsingType behavior. It has a 9-line comment!)


================
Comment at: clang/lib/AST/TemplateName.cpp:115
 
+  if (auto *USD = Storage.dyn_cast<UsingShadowDecl *>())
+    return cast<TemplateDecl>(USD->getTargetDecl());
----------------
`= getAsUsingShadowDecl`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:1114
+
       if (SS.isNotEmpty())
         Template =
----------------
FIXME here for qualified using case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123127