[PATCH] D123775: [AST] Support template declaration found through using-decl for QualifiedTemplateName.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/AST/PropertiesBase.td:666
   }
-  def : Property<"declaration", TemplateDeclRef> {
+  def : Property<"templateDecl", TemplateDeclRef> {
     let Read = [{ qtn->getTemplateDecl() }];
----------------
this seems to be flattening the TemplateName into its possible attributes.

Is it possible to encode it as a single TemplateName property? are there particular advantages/disadvantages?


================
Comment at: clang/include/clang/AST/TemplateName.h:418
+  /// The underlying template name, it is either
+  //  1) a Template TemplateName -- a template declaration or set of overloaded
+  //     function templates that this qualified name refers to.
----------------
I'm confused about the overloaded case: before this patch it was a single TemplateDecl. Have we actually added the possibility that UnderlyingTemplate is an Overload in this patch? Where?


================
Comment at: clang/include/clang/AST/TemplateName.h:418
+  /// The underlying template name, it is either
+  //  1) a Template TemplateName -- a template declaration or set of overloaded
+  //     function templates that this qualified name refers to.
----------------
sammccall wrote:
> I'm confused about the overloaded case: before this patch it was a single TemplateDecl. Have we actually added the possibility that UnderlyingTemplate is an Overload in this patch? Where?
nit: "Template TemplateName" reads a little strangely or reminds of TemplateTemplateParm
I think "a Template" is clear in context. (and below)


================
Comment at: clang/include/clang/AST/TemplateName.h:426
+                        TemplateName Template)
+      : Qualifier(NNS, TemplateKeyword ? 1 : 0), UnderlyingTemplate(Template) {}
 
----------------
you've documented the invariant above, probably makes sense to assert it here


================
Comment at: clang/include/clang/AST/TemplateName.h:444
+  /// declaration, returns the using-shadow declaration.
+  UsingShadowDecl *getUsingShadowDecl() const {
+    return UnderlyingTemplate.getAsUsingShadowDecl();
----------------
It seems all of the callers of this function ultimately just use it to reconstruct the underlying template name. This makes sense, because you mostly don't look at QualifiedTemplateName unless you're handling every TemplateName case, and normally handle it by recursion.

I think:
- we should definitely expose the underlying TemplateName and many callers should use that
- we should probably remove getUsingShadowDecl() unless there are enough callers that benefit from not getting it via TemplateName
- we should *consider* removing getTemplateDecl(), it makes for a simpler logical model (basically the top TemplateName::getTemplateName() decl does this smart unwrapping of sugar, but the specific representations like QualifiedTemplateName just expose their components). If it's too much of a pain for callers, we don't need to do this of course.


================
Comment at: clang/lib/AST/ASTContext.cpp:4854
   // Look through qualified template names.
-  if (QualifiedTemplateName *QTN = Template.getAsQualifiedTemplateName())
-    Template = TemplateName(QTN->getTemplateDecl());
+  if (QualifiedTemplateName *QTN = Template.getAsQualifiedTemplateName()) {
+    if (UsingShadowDecl *USD = QTN->getUsingShadowDecl())
----------------
This seems like an obvious example where we want:
```
if (QTN = ...)
  Template = QTN->getUnderlyingTemplate();
```


================
Comment at: clang/unittests/AST/TemplateNameTest.cpp:92
+    namespace absl { using std::vector; }
+    // absl::vector is a TemplateSpecializationType with an inner Using
+    // TemplateName (not a Qualified TemplateName, the qualifiers are
----------------
absl::vector -> absl::vector<int>

is a TemplateSpecializationType -> is an elaborated TemplateSpecializationType
(I think the TST itself just covers the vector<int> part)


================
Comment at: clang/unittests/AST/TemplateNameTest.cpp:93
+    // absl::vector is a TemplateSpecializationType with an inner Using
+    // TemplateName (not a Qualified TemplateName, the qualifiers are
+    // stripped when constructing the TemplateSpecializationType)!
----------------
the qualifiers are rather part of the ElaboratedTypeLoc


================
Comment at: clang/unittests/AST/TemplateNameTest.cpp:97
+  )cpp");
+  auto Matcher = typeLoc(loc(templateSpecializationType().bind("id")));
+  auto MatchResults = match(Matcher, AST->getASTContext());
----------------
Describe more of the structure?

elaboratedTypeLoc(hasNamedTypeLoc(loc(templateSpecializationType())))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123775