hokein added a comment.
In D123127#3429589 <https://reviews.llvm.org/D123127#3429589>, @sammccall wrote:
> This looks pretty good!
> The tests in clang are sadly indirect.
This is mostly due to the fact that `TemplateName` is not dumped. This patch
extend the `NodeDumper` to print a `using` indicator, I think it is relatively
obvious in the AST.
> 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.
Adding support in clangd `FindTarget` is my next step, it would be a followup
patch. I feel like the FindTarget tests are more related to the application
logic of the new `TemplateName`, and are less direct compared with the tests in
this patch.
================
Comment at: clang/include/clang/AST/TemplateName.h:446
/// that this qualified name refers to.
TemplateDecl *Template;
----------------
sammccall wrote:
> 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)
> 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...)
Agree, I think this is the right solution, but would require more work (a
followup patch).
There are two options, 1) leave the qualified template name out, and support it
in a followup patch, 2) workaround it -- we wrap the qualified template name
with a using template name, though it doesn't quite match our mental model, I
think it is probably ok. (I added a FIXME in `SemaTemplate.cpp`).
Let me know what you think, also happy to switch to 1).
================
Comment at: clang/lib/AST/ASTContext.cpp:6176
}
+ case TemplateName::UsingTemplate:
+ return getCanonicalTemplateName(
----------------
sammccall wrote:
> you could also handle this in the QualifiedTemplate/Template case, since the
> decl is guaranteed to be present.
yeah, but I think the current version is clearer.
================
Comment at: clang/lib/AST/TemplateName.cpp:273
+ } else if (auto *U = getAsUsingTemplateName()) {
+ U->getUnderlying().print(OS, Policy);
} else {
----------------
sammccall wrote:
> 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.
>
good point, you're right, this will print duplicated nested specifiers. Change
it to print unqualified name only, and added a test.
================
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) &&
----------------
sammccall wrote:
> 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
yeah, it prevents a bogus diagnostic introduced by this patch.
```
namespace N {
template<typename T> struct NamedNS1 {}; // expected-note {{here}}
}
using N::NamedNS1;
NamedNS1(int) -> NamedNS1<int>; // expected-error {{deduction guide must be
declared in the same scope as template}}
```
With this patch without the change here, we emit an extra diagnostic `deduced
type 'NamedNS1<int>' of deduction guide is not written as a specialization of
template 'NamedNS1'`.
And this part of code might have another issue where it emits a wrong
`err_deduction_guide_defines_function` for a correct code, (this is something I
plan to take a look but not in this patch) .
================
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
----------------
sammccall wrote:
> revert?
>
> (though yes, this is annoying)
oops, an accident change, reverted.
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
