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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/include/clang/AST/PropertiesBase.td:666
   }
-  def : Property<"declaration", TemplateDeclRef> {
+  def : Property<"templateDecl", TemplateDeclRef> {
     let Read = [{ qtn->getTemplateDecl() }];
----------------
sammccall wrote:
> 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?
use `TemplateName` now. 


================
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:
> 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)
> before this patch it was a single TemplateDecl. Have we actually added the possibility that UnderlyingTemplate is an Overload in this patch? Where?

No, this was basically moved from the original comment, but you're right, the comment doesn't reflect the current code, I think it was oversight when we abstracted out a QualifiedTemplateName in https://github.com/llvm/llvm-project/commit/d28ae27d8d2e20672e182710160f27cf821b55fd. Removed it.


================
Comment at: clang/include/clang/AST/TemplateName.h:444
+  /// declaration, returns the using-shadow declaration.
+  UsingShadowDecl *getUsingShadowDecl() const {
+    return UnderlyingTemplate.getAsUsingShadowDecl();
----------------
sammccall wrote:
> 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.
> we should definitely expose the underlying TemplateName and many callers should use that

+1, this sounds good, and it can also simplify some user code.

> we should *consider* removing getTemplateDecl()

This is doable, there are not too many callers of this API (~14 in-tree occurrences), but I'd prefer to do it in a follow-up patch. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123775