This is a followup of https://reviews.llvm.org/D123127, adding support
for the QualifiedTemplateName.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | |
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:
| |
clang/lib/AST/ASTContext.cpp | ||
4855 | 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 | |
93 | the qualifiers are rather part of the ElaboratedTypeLoc | |
97 | Describe more of the structure? elaboratedTypeLoc(hasNamedTypeLoc(loc(templateSpecializationType()))) |
format
clang/include/clang/AST/PropertiesBase.td | ||
---|---|---|
665–666 | use TemplateName now. | |
clang/include/clang/AST/TemplateName.h | ||
418 |
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 |
+1, this sounds good, and it can also simplify some user code.
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. |
clang/lib/AST/ASTImporter.cpp | ||
---|---|---|
9189 | why not just import(UnderlyingTN) directly, instead of importing its shadowdecl or templatedecl case-by-case? |
simplify the code in ASTImporter.
clang/lib/AST/ASTImporter.cpp | ||
---|---|---|
9189 | good catch, not sure how I missed this. |
clang/lib/Sema/SemaTemplate.cpp | ||
---|---|---|
1004 | nitpick: /*HasTemplateKeyword*/ -> /*HasTemplateKeyword=*/ |
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?