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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/AST/TemplateName.cpp:80
 TemplateName::TemplateName(DependentTemplateName *Dep) : Storage(Dep) {}
+TemplateName::TemplateName(UsingShadowDecl *Using) : Storage(Using) {}
 
----------------
sammccall wrote:
> assert(isa<TemplateDecl>(Using->getTargetDecl()))
> 
> (better to crash upfront than in getAsTemplateDecl() or so)
There is already a stricter assertion `TD == Using->getUnderlyingDecl()` in every construct side.


================
Comment at: clang/lib/AST/TemplateName.cpp:246
                  TemplateNameDependenceScope::DependentInstantiation)
       Template->printQualifiedName(OS, Policy);
     else
----------------
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`.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:292
+          Context.getQualifiedTemplateName(Qualifier, hasTemplateKeyword, TD);
+    } else if (!R.isAmbiguous() && R.begin()->getKind() == Decl::UsingShadow) {
+      UsingShadowDecl *Using = cast<UsingShadowDecl>(*R.begin());
----------------
sammccall wrote:
> it would be really nice to have a test verifying that if we are ambiguous here and form an Overloaded TemplateName, then when it's later resolved it turns into a Using TemplateName where appropriate...
> then when it's later resolved it turns into a Using TemplateName where appropriate...

clang doesn't seem to work like this -- it does form an Overloaded TemplateName during parsing, but a call to these overloaded function templates is a UnresolvedLookupExpr which stores all function candidates. After the template instantiation, the UnresolvedLookupExpr  is resolved to a normal DeclRefExpr (that said there is no Using TemplateName being built).

The reason I added the `isAmbiguous` guard here was to ensure the `Using->getUnderlyingDecl() == TD` invariant, it is always true for correct cases. However, we pickup an *arbitrary* template for error-recovery (see line 231). Now I have figured out a better way to handle it (by storing the corresponding FoundUsingDecl) without needing this `isAmbiguous` guard. Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123127