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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Storing the UsingShadowDecl directly is much nicer :-)



================
Comment at: clang/include/clang/AST/TemplateName.h:231
+
+    /// A template name that refers to a template declaration through a specific
+    /// using shadow declaration.
----------------
nit: declaration *found* through


================
Comment at: clang/include/clang/AST/TemplateName.h:302
+  /// can be retrieved via the using shadow declaration.
+  UsingShadowDecl *getAsUsingDecl() const;
+
----------------
getAsUsing**Shadow**Decl!



================
Comment at: clang/lib/AST/ASTContext.cpp:6129
+  case TemplateName::UsingTemplate: {
+    return DeclarationNameInfo(Name.getAsUsingDecl()->getDeclName(), NameLoc);
+  }
----------------
nit: no braces (other cases only use them where variables are declared)


================
Comment at: clang/lib/AST/ASTImporter.cpp:9194
+  case TemplateName::UsingTemplate: {
+    // auto *UTN = From.getAsUsingTemplateName();
+    auto UsingOrError = Import(From.getAsUsingDecl());
----------------
remove commented code


================
Comment at: clang/lib/AST/ASTImporter.cpp:9201
+      return ToTemplateOrErr.takeError();
+    return TemplateName(cast<TemplateDecl>(*ToTemplateOrErr));
+  }
----------------
This doesn't look right at all, you're returning an ordinary template name, not a using...


================
Comment at: clang/lib/AST/TemplateName.cpp:80
 TemplateName::TemplateName(DependentTemplateName *Dep) : Storage(Dep) {}
+TemplateName::TemplateName(UsingShadowDecl *Using) : Storage(Using) {}
 
----------------
assert(isa<TemplateDecl>(Using->getTargetDecl()))

(better to crash upfront than in getAsTemplateDecl() or so)


================
Comment at: clang/lib/AST/TemplateName.cpp:246
                  TemplateNameDependenceScope::DependentInstantiation)
       Template->printQualifiedName(OS, Policy);
     else
----------------
this is going to print the wrong qualifier: qualifier of the underlying rather than of the usingdecl


================
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());
----------------
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123127