rsmith added a comment. In D78404#1990461 <https://reviews.llvm.org/D78404#1990461>, @broadwaylamb wrote:
> In D78404#1990192 <https://reviews.llvm.org/D78404#1990192>, @rsmith wrote: > > > ... please also test ... > > > > template<typename T> class TemplateClass3<T, &TestClass::func> > > varTemplate3{}; > > > > > > ... which we should diagnose, because that's a primary variable template > > definition, not a partial / explicit specialization or explicit > > instantiation. > > > Interestingly, the latest GCC doesn't diagnose here > <https://godbolt.org/z/svejsJ>, but we do. Do we need to remain compatible > with GCC in this case? GCC doesn't even diagnose class TestClass { struct X {}; }; template<typename T> TestClass::X varTemplate3_1b{}; void use() { varTemplate3_1b<int> = {}; } ... which is a flagrant access violation. I don't think we should be compatible with that, it just seems like a regular GCC bug rather than any kind of intentional extension. ================ Comment at: clang/include/clang/AST/Decl.h:3198 /// alias-declaration. -class TypeAliasDecl : public TypedefNameDecl { +class TypeAliasDecl : public TypedefNameDecl, public DeclContext { /// The template for which this is the pattern, if any. ---------------- broadwaylamb wrote: > I'm not sure about inheriting `TypeAliasDecl` from `DeclContext`, but (see > below) Yeah, this doesn't seem appropriate. Hopefully we can find another way. ================ Comment at: clang/lib/Parse/ParseDecl.cpp:5672 + // initializer. + SuppressAccessChecks diagsFromTag(*this); + ---------------- broadwaylamb wrote: > This is for things like > > ``` > template<> void X<Y::Z>::f() {} > ``` > > not to be rejected (here `Z` is a private member of class `Y`) > > I wasn't sure how to suppress it only when we're parsing template parameter > list, so we suppress it unconditionally here. All the tests pass though, but > I'd appreciate any hints. > > Note that testing that `D.getContext() == > DeclaratorContext::TemplateParamContext` doesn't work — when we get here, > we're actually in a `FileContext`. The cases that this is going to incorrectly accept will be a bit weird. For example: ``` struct A { void f(); }; class B { using T = A; }; void B::T::f() {} ``` We should be able to feed through information on whether we're parsing after `template` or `template<>` here (in which case we should suppress access) -- it seems reasonable to track that on the `Declarator`. One thing that seems unclear is whether we should suppress access here: ``` template<typename T> struct A; class X { struct Y {}; }; template<> struct A<X::Y> { void f(); }; void A<X::Y>::f() {} // suppress access here or not? ``` To get that right, we'd need to temporarily suppress access here and do a one-token lookahead past the scope specifier before diagnosing -- this case should still report an access error: ``` template<typename T> struct A; class X { struct Y {}; }; template<> struct A<X::Y> { int f(); }; int A<X::Y>::*f() {} // do not suppress access here, this is a regular non-template function ``` > Note that testing that `D.getContext() == > DeclaratorContext::TemplateParamContext` doesn't work Right, that context indicates that we're parsing a template parameter. ================ Comment at: clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp:58 +template <typename T> +using alias3_1 = TemplateClass3<T, &TestClass::func>; + ---------------- broadwaylamb wrote: > I'm talking about declarations like this. > > Previously, we didn't reject it (which I believe was incorrect), and now we > do. Do you know where we're suppressing the diagnostic in the first place? After we parse the *template-head*, we may be able to just look at the next token (to see if it's `using`) to determine if we should defer access checks. Making `TypeAliasDecl` be a `DeclContext` seems like a very heavy-handed way of handling this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78404/new/ https://reviews.llvm.org/D78404 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits