[PATCH] D92024: [clang] Implement P0692R1 from C++20 (access checking on specializations and instantiations)

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:265
+  // We don't use `SuppressAccessChecks` here because
+  // it supresses ALL the dignostics kinds, but we need to keep some of them.
+  // For instance marked as unavailable:
----------------
Spelling: /supresses/suppresses/, /dignostics kinds/kinds of diagnostics/


================
Comment at: clang/test/CXX/class.access/class.friend/p1.cpp:290
   A::I f2(A::I i = A::x) {} // expected-error 3 {{is a private member of}}
-  template<typename T> A::I g2(A::I i) { // expected-error 2 {{is a private member of}}
+  template <typename T> A::I g2(A::I i) { // expected-error {{is a private member of}}
     T t;
----------------
This whitespace diff is gratuitous.


================
Comment at: clang/test/CXX/class.access/class.friend/p1.cpp:294
+  template <> A::I g2<char>(A::I i) { return 0; } // expected-error {{is a private member of}}
   template A::I g2<A::I>(A::I i);
 }
----------------
IMHO it would make sense to add several other variations here too:

```
template <> A::I g2<char>(A::I i); // expected-error {{is a private member of}}
template <> int g2<A::I>(int i);  // expected-error??
template <> A::I g2<A::I*>(A::I i);  // expected-error??
template A::I g2<char>(A::I i); // expected-error {{is a private member of}}
template int g2<A::I**>(int i);  // OK
template A::I g2<A::I***>(A::I i);  // OK??
```

It might also be better to use a well-formed template `template<class> int g3(int) { return 0; }` instead of `g2` for the explicit specializations, and to use yet another template `template<class> int g4(int) { return 0; }` instead of `g2` for the explicit instantiation declarations. That way there's no accidental "cross-talk" between the various declarations that might affect what diagnostics get printed.


================
Comment at: clang/test/CXX/temp/temp.spec/part.spec.cpp:10
+
+// class for tests
+class TestClass {
----------------
I wonder if this test can be shortened and/or removed, now that you're not doing weird parser tricks that need exhaustive testing. E.g. at least the stuff with the `final` keyword could be removed now, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92024/new/

https://reviews.llvm.org/D92024