hokein added inline comments.
================
Comment at: clang/include/clang/AST/TemplateName.h:446
/// that this qualified name refers to.
TemplateDecl *Template;
----------------
sammccall wrote:
> hokein wrote:
> > 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).
> I don't like 2), it adds code in order to introduce an incorrect "inside-out"
> AST in a surprising way that will lead to subtle bugs, and we'll need to undo
> this later. To me this seems probably worse than dropping one of the sugar
> nodes and never fixing it, which is already pretty bad.
>
> I think we should do 1) instead - I think it's actually simpler to land the
> QualifiedTemplate-holds-TemplateName patch *first*, but either order seems
> fine.
ok, faire enough, switched to 1).
================
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:
> hokein wrote:
> > 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) .
> That makes sense, though it seems to me inconsistent to suppress the
> secondary diagnostic for UsingTemplate but emit it for QualifiedTemplate -
> it's equally bogus/valid in both cases.
>
> Can we either suppress both, or suppress neither for now and suppress them in
> a subsequent patch?
ok, let's not do it for now, and add a FIXME for the newly-introduced
diagnostic in the existing test.
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
