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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
