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

This looks pretty good!
The tests in clang are sadly indirect.
I think adding support to clangd's FindTarget would be a small change and would allow a fairly direct test, but maybe it will affect a bunch of existing tests or possibly have a blast radius. Up to you.



================
Comment at: clang/include/clang/AST/ASTContext.h:2184
 
+  TemplateName getUsingTemplateName(TemplateName Underlying,
+                                    UsingShadowDecl *USD) const;
----------------
nit: there's something of a canonical order in TemplateName::NameKind, I think it's useful to follow this as much as possible for consistency when navigating in these huge files.
This mostly means placing UsingTemplate code at the bottom of lists of template name cases.


================
Comment at: clang/include/clang/AST/TemplateName.h:29
 class DependentTemplateName;
+class UsingTemplateName;
 class IdentifierInfo;
----------------
this list is alpha sorted, please preserve (and below)


================
Comment at: clang/include/clang/AST/TemplateName.h:411
+public:
+  UsingShadowDecl *getUsingShadowDecl() const { return USD; }
+  TemplateName getUnderlying() const { return UnderlyingTN; }
----------------
docs on getUsingShadowDecl and getUnderlying


================
Comment at: clang/include/clang/AST/TemplateName.h:411
+public:
+  UsingShadowDecl *getUsingShadowDecl() const { return USD; }
+  TemplateName getUnderlying() const { return UnderlyingTN; }
----------------
sammccall wrote:
> docs on getUsingShadowDecl and getUnderlying
getFoundDecl() would be consistent with UsingType, and would describe the relation between this and the UsingShadowDecl, rather than echoing its type


================
Comment at: clang/include/clang/AST/TemplateName.h:412
+  UsingShadowDecl *getUsingShadowDecl() const { return USD; }
+  TemplateName getUnderlying() const { return UnderlyingTN; }
+  void Profile(llvm::FoldingSetNodeID &ID) { Profile(ID, UnderlyingTN, USD); }
----------------
maybe getUnderlyingTemplate? "underlying" acts as an adjective so this feels a little ungrammatical


================
Comment at: clang/include/clang/AST/TemplateName.h:413
+  TemplateName getUnderlying() const { return UnderlyingTN; }
+  void Profile(llvm::FoldingSetNodeID &ID) { Profile(ID, UnderlyingTN, USD); }
+  static void Profile(llvm::FoldingSetNodeID &ID, TemplateName Underlying,
----------------
nit: separate the public API from implementation details with a blank line at least


================
Comment at: clang/include/clang/AST/TemplateName.h:446
   /// that this qualified name refers to.
   TemplateDecl *Template;
 
----------------
It seems really unfortunate that this is a `TemplateDecl*` instead of a `TemplateName`.

for:
```
template <int> class x;
namespace a { using ::x; }
a::x<0>;
```
we want `a::x` to include both a QualifiedTemplateName (modelling the a:: qualifier) and a UsingTemplateName (modelling the UsingShadowDecl that was found).

I'd guess we can change `Template` to be a `TemplateName` internally, and add `getTemplate()` while keeping the existing `get[Template]Decl` APIs on top of `TemplateName::getAsTemplateDecl()`.
I suppose this could be a separate change (though it'd be nice to know whether it's going to work...)

Of course if that changed there's a natural followon to take the qualifier out of DependentTemplateName and turn it into a QualifiedTemplateName wrapping a DependentTemplateName. (Definitely out of scope for this patch, not sure if it's reasonable to expect you to do it at all)


================
Comment at: clang/lib/AST/ASTContext.cpp:6176
   }
+  case TemplateName::UsingTemplate:
+    return getCanonicalTemplateName(
----------------
you could also handle this in the QualifiedTemplate/Template case, since the decl is guaranteed to be present.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:2394
+      TN = TN.getAsUsingTemplateName()->getUnderlying();
+      goto mangleTemplateName;
+    }
----------------
this seems pretty scary: we have to use a goto here rather than extract a function, because the target code also uses goto...

The nontrivial TemplateTemplateParmDecl case isn't possible here. So I suggest just calling mangleSourceNameWithAbiTags directly without trying to reuse logic.


================
Comment at: clang/lib/AST/TemplateName.cpp:107
     return Template;
-
+  if (auto *UTN = Storage.dyn_cast<UsingTemplateName *>())
+    return UTN->getUnderlying().getAsTemplateDecl();
----------------
`= getAsUsingTemplateName()`


================
Comment at: clang/lib/AST/TemplateName.cpp:273
+  } else if (auto *U = getAsUsingTemplateName()) {
+    U->getUnderlying().print(OS, Policy);
   } else {
----------------
Why is this correct, rather than potentially printing the wrong sugar?

If the reasoning is that that the underlying TemplateName has the same name and can't have any unwanted sugar, that seems subtle and deserves a comment.

I think just printing the unqualified name directly would be clearer.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:506
     if (auto *TD = getAsTypeTemplateDecl(IIDecl)) {
-      // FIXME: TemplateName should include FoundUsingShadow sugar.
-      T = Context.getDeducedTemplateSpecializationType(TemplateName(TD),
-                                                       QualType(), false);
+      TemplateName Template = TemplateName(TD);
+      if (FoundUsingShadow)
----------------
nit: just Template(TD) or = TD?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11026
           Context.hasSameTemplateName(SpecifiedName, GuidedTemplate);
-      if (SpecifiedName.getKind() == TemplateName::Template && TemplateMatches)
+      if ((SpecifiedName.getKind() == TemplateName::Template ||
+           SpecifiedName.getKind() == TemplateName::UsingTemplate) &&
----------------
I don't think it can be valid to find the template name via a usingshadowdecl, because the deduction guide must be declared in the same scope as the template. (err_deduction_guide_wrong_scope).

Is this to prevent an unneccesary second diagnostic? (And if so, I wonder whether we should also include QualifiedTemplate, maybe as a separate change). It may deserve a comment


================
Comment at: clang/lib/Tooling/CMakeLists.txt:62
         # Skip this in debug mode because parsing AST.h is too slow
-        --skip-processing=${skip_expensive_processing}
+        --skip-processing=1
         -I ${LLVM_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
----------------
revert?

(though yes, this is annoying)


================
Comment at: clang/test/AST/ast-dump-using-template.cpp:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++17 -ast-dump %s | FileCheck -strict-whitespace %s
+
----------------
Maybe a comment here:
TemplateNames are not dumped, so the sugar here isn't obvious. However the "using" on the TemplateSpecializationTypes shows that the UsingTemplateName is present.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123127