sammccall added a comment.
This looks pretty good!
The tests in clang are sadly indirect.
I think adding support to clangd's FindTarget would be a small change and would
allow a fairly direct test, but maybe it will affect a bunch of existing tests
or possibly have a blast radius. Up to you.
================
Comment at: clang/include/clang/AST/ASTContext.h:2184
+ TemplateName getUsingTemplateName(TemplateName Underlying,
+ UsingShadowDecl *USD) const;
----------------
nit: there's something of a canonical order in TemplateName::NameKind, I think
it's useful to follow this as much as possible for consistency when navigating
in these huge files.
This mostly means placing UsingTemplate code at the bottom of lists of template
name cases.
================
Comment at: clang/include/clang/AST/TemplateName.h:29
class DependentTemplateName;
+class UsingTemplateName;
class IdentifierInfo;
----------------
this list is alpha sorted, please preserve (and below)
================
Comment at: clang/include/clang/AST/TemplateName.h:411
+public:
+ UsingShadowDecl *getUsingShadowDecl() const { return USD; }
+ TemplateName getUnderlying() const { return UnderlyingTN; }
----------------
docs on getUsingShadowDecl and getUnderlying
================
Comment at: clang/include/clang/AST/TemplateName.h:411
+public:
+ UsingShadowDecl *getUsingShadowDecl() const { return USD; }
+ TemplateName getUnderlying() const { return UnderlyingTN; }
----------------
sammccall wrote:
> docs on getUsingShadowDecl and getUnderlying
getFoundDecl() would be consistent with UsingType, and would describe the
relation between this and the UsingShadowDecl, rather than echoing its type
================
Comment at: clang/include/clang/AST/TemplateName.h:412
+ UsingShadowDecl *getUsingShadowDecl() const { return USD; }
+ TemplateName getUnderlying() const { return UnderlyingTN; }
+ void Profile(llvm::FoldingSetNodeID &ID) { Profile(ID, UnderlyingTN, USD); }
----------------
maybe getUnderlyingTemplate? "underlying" acts as an adjective so this feels a
little ungrammatical
================
Comment at: clang/include/clang/AST/TemplateName.h:413
+ TemplateName getUnderlying() const { return UnderlyingTN; }
+ void Profile(llvm::FoldingSetNodeID &ID) { Profile(ID, UnderlyingTN, USD); }
+ static void Profile(llvm::FoldingSetNodeID &ID, TemplateName Underlying,
----------------
nit: separate the public API from implementation details with a blank line at
least
================
Comment at: clang/include/clang/AST/TemplateName.h:446
/// that this qualified name refers to.
TemplateDecl *Template;
----------------
It seems really unfortunate that this is a `TemplateDecl*` instead of a
`TemplateName`.
for:
```
template <int> class x;
namespace a { using ::x; }
a::x<0>;
```
we want `a::x` to include both a QualifiedTemplateName (modelling the a::
qualifier) and a UsingTemplateName (modelling the UsingShadowDecl that was
found).
I'd guess we can change `Template` to be a `TemplateName` internally, and add
`getTemplate()` while keeping the existing `get[Template]Decl` APIs on top of
`TemplateName::getAsTemplateDecl()`.
I suppose this could be a separate change (though it'd be nice to know whether
it's going to work...)
Of course if that changed there's a natural followon to take the qualifier out
of DependentTemplateName and turn it into a QualifiedTemplateName wrapping a
DependentTemplateName. (Definitely out of scope for this patch, not sure if
it's reasonable to expect you to do it at all)
================
Comment at: clang/lib/AST/ASTContext.cpp:6176
}
+ case TemplateName::UsingTemplate:
+ return getCanonicalTemplateName(
----------------
you could also handle this in the QualifiedTemplate/Template case, since the
decl is guaranteed to be present.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:2394
+ TN = TN.getAsUsingTemplateName()->getUnderlying();
+ goto mangleTemplateName;
+ }
----------------
this seems pretty scary: we have to use a goto here rather than extract a
function, because the target code also uses goto...
The nontrivial TemplateTemplateParmDecl case isn't possible here. So I suggest
just calling mangleSourceNameWithAbiTags directly without trying to reuse logic.
================
Comment at: clang/lib/AST/TemplateName.cpp:107
return Template;
-
+ if (auto *UTN = Storage.dyn_cast<UsingTemplateName *>())
+ return UTN->getUnderlying().getAsTemplateDecl();
----------------
`= getAsUsingTemplateName()`
================
Comment at: clang/lib/AST/TemplateName.cpp:273
+ } else if (auto *U = getAsUsingTemplateName()) {
+ U->getUnderlying().print(OS, Policy);
} else {
----------------
Why is this correct, rather than potentially printing the wrong sugar?
If the reasoning is that that the underlying TemplateName has the same name and
can't have any unwanted sugar, that seems subtle and deserves a comment.
I think just printing the unqualified name directly would be clearer.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:506
if (auto *TD = getAsTypeTemplateDecl(IIDecl)) {
- // FIXME: TemplateName should include FoundUsingShadow sugar.
- T = Context.getDeducedTemplateSpecializationType(TemplateName(TD),
- QualType(), false);
+ TemplateName Template = TemplateName(TD);
+ if (FoundUsingShadow)
----------------
nit: just Template(TD) or = TD?
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11026
Context.hasSameTemplateName(SpecifiedName, GuidedTemplate);
- if (SpecifiedName.getKind() == TemplateName::Template && TemplateMatches)
+ if ((SpecifiedName.getKind() == TemplateName::Template ||
+ SpecifiedName.getKind() == TemplateName::UsingTemplate) &&
----------------
I don't think it can be valid to find the template name via a usingshadowdecl,
because the deduction guide must be declared in the same scope as the template.
(err_deduction_guide_wrong_scope).
Is this to prevent an unneccesary second diagnostic? (And if so, I wonder
whether we should also include QualifiedTemplate, maybe as a separate change).
It may deserve a comment
================
Comment at: clang/lib/Tooling/CMakeLists.txt:62
# Skip this in debug mode because parsing AST.h is too slow
- --skip-processing=${skip_expensive_processing}
+ --skip-processing=1
-I ${LLVM_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
----------------
revert?
(though yes, this is annoying)
================
Comment at: clang/test/AST/ast-dump-using-template.cpp:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++17 -ast-dump %s |
FileCheck -strict-whitespace %s
+
----------------
Maybe a comment here:
TemplateNames are not dumped, so the sugar here isn't obvious. However the
"using" on the TemplateSpecializationTypes shows that the UsingTemplateName is
present.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123127/new/
https://reviews.llvm.org/D123127
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
