[PATCH] D78404: [clang] Implement P0692R1 from C++20 (access checking on specializations)
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Apr 18 18:52:06 PDT 2020
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
More information about the cfe-commits
mailing list