[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 a comment.

Nice fix! Just nits



================
Comment at: clang/include/clang/AST/TemplateName.h:197
   using StorageType =
-      llvm::PointerUnion<TemplateDecl *, UncommonTemplateNameStorage *,
+      llvm::PointerUnion<NamedDecl *, UncommonTemplateNameStorage *,
                          QualifiedTemplateName *, DependentTemplateName *>;
----------------
nit: I'd use Decl here instead of NamedDecl.

Sure, the lowest common type is NamedDecl, but that's not really important and we don't use any of the NamedDecl interface.


================
Comment at: clang/include/clang/AST/TemplateName.h:303
+  ///
+  /// The underlying template declaration is not stored in the template name, it
+  /// can be retrieved via the using shadow declaration.
----------------
I think this second sentence is confusing rather than helpful to understanding to use this class (as opposed to its implementation).

The main way to get the underlying declaration is with `getAsTemplateDecl()`. From a public POV, a Using name has a using shadow decl *and* a template decl, the fact that we only store one of these is an implementation detail.


================
Comment at: clang/include/clang/AST/TemplateName.h:305
+  /// can be retrieved via the using shadow declaration.
+  UsingShadowDecl *getAsUsingShadowDecl() const;
+
----------------
If (after followup patches) you have a qualified name Q wrapping a using name U, I'd expect Q.getAsUsingShadowDecl() to extract the pointer from U.storage.

I'd expect this based on the first sentence of the comment here, and also consistent with getAsTemplateDecl() which does this sort of unwrapping.

So no changes needed if you're OK with adding that unwrapping behavior in the next patch, otherwise I think we need to think a bit about how to set expectations with this API.


================
Comment at: clang/lib/AST/TemplateName.cpp:86
+  if (auto *ND = Storage.dyn_cast<NamedDecl *>()) {
+    if (isa<TemplateDecl>(ND))
+      return Template;
----------------
nit: invert this so that UsingShadowDecl is the special case and TemplateDecl the general one?


================
Comment at: clang/lib/AST/TemplateName.cpp:112
+      return TD;
+    assert(isa<UsingShadowDecl>(TemplateOrUsing));
+    return cast<TemplateDecl>(
----------------
and again here


================
Comment at: clang/lib/AST/TemplateName.cpp:256
+    //
+    // Similar to the UsingType behavior, std::vector is much more common, and
+    // provides more information in practice, we print the underlying template
----------------
I don't understand what "std::vector is much more common" there's no ground truth for us to observe.
Rather maybe "using declarations are used to import names more often than to export them, and using the original name is most useful in this case"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123127