This is an archive of the discontinued LLVM Phabricator instance.

[AST] Support template declaration found through using-decl for QualifiedTemplateName.
ClosedPublic

Authored by hokein on Apr 14 2022, 2:47 AM.

Details

Summary

This is a followup of https://reviews.llvm.org/D123127, adding support
for the QualifiedTemplateName.

Diff Detail

Event Timeline

hokein created this revision.Apr 14 2022, 2:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: martong. · View Herald Transcript
hokein requested review of this revision.Apr 14 2022, 2:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 2:47 AM
sammccall added inline comments.Apr 14 2022, 6:15 AM
clang/include/clang/AST/PropertiesBase.td
665–666

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?

clang/include/clang/AST/TemplateName.h
418

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?

418

nit: "Template TemplateName" reads a little strangely or reminds of TemplateTemplateParm
I think "a Template" is clear in context. (and below)

426

you've documented the invariant above, probably makes sense to assert it here

450

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.
clang/lib/AST/ASTContext.cpp
4862

This seems like an obvious example where we want:

if (QTN = ...)
  Template = QTN->getUnderlyingTemplate();
clang/unittests/AST/TemplateNameTest.cpp
92

absl::vector -> absl::vector<int>

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

93

the qualifiers are rather part of the ElaboratedTypeLoc

97

Describe more of the structure?

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

hokein updated this revision to Diff 423686.Apr 19 2022, 11:20 AM
hokein marked 6 inline comments as done.

expose underlying template name, removed the getUsingShadowDecl;
address comments;

hokein updated this revision to Diff 423687.Apr 19 2022, 11:23 AM

format

clang/include/clang/AST/PropertiesBase.td
665–666

use TemplateName now.

clang/include/clang/AST/TemplateName.h
418

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.

450

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.

sammccall accepted this revision.Apr 19 2022, 11:36 AM
sammccall added inline comments.
clang/lib/AST/ASTImporter.cpp
9189

why not just import(UnderlyingTN) directly, instead of importing its shadowdecl or templatedecl case-by-case?

This revision is now accepted and ready to land.Apr 19 2022, 11:36 AM
hokein updated this revision to Diff 423843.Apr 20 2022, 2:09 AM
hokein marked an inline comment as done.

simplify the code in ASTImporter.

clang/lib/AST/ASTImporter.cpp
9189

good catch, not sure how I missed this.

shafik added inline comments.Apr 20 2022, 4:52 PM
clang/lib/Sema/SemaTemplate.cpp
1003–1004

nitpick: /*HasTemplateKeyword*/ -> /*HasTemplateKeyword=*/