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

[Alex Orlov via Phabricator via cfe-commits]

aorlov added inline comments.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:172
+
+  // TODO. This can produce wrong detection in case of a later class
+  // declaration. Example:
----------------
Quuxplusone wrote:
> I don't know the purpose of this code, but this //seems// like a super important TODO.
> 
> The comment "look ahead until `{`" is also scary because
> ```
> template<int I, int J> struct A {};
> template<int J> struct A<int{}, J> {};
> ```
> is also a partial specialization. Why do we need to know `isSpecialization`, who's //supposed// to compute it, and why does their answer need to be fixed-up right here?
> 
> And is this specific piece of the patch perhaps severable into a separate review? Again, I don't know this code, but... It seems like you've got one part of the patch — "add a `SuppressAccessGuard` around the call to `ParseDeclarator`" — which is either a 100% correct or 100% incorrect implementation of P0692R1. And then you've got this other piece, a parser hack, which looks much more heuristic and incomplete in nature, and looks orthogonal to P0692R1.
> 
> Btw, I'm not an approver on this (and shouldn't be); if you want better review you might want to ping someone who's touched this code lately (according to `git blame`).
@Quuxplusone
> I don't know the purpose of this code, but this seems like a super important TODO.
This is a deprecated hint, I'll remove it.
> The comment "look ahead until {" is also scary 
Nice case! I'll improve the patch.
> Why do we need to know isSpecialization
This flag helps to turn off usual access rules further:
```
Parser::ParseClassSpecifier(... const ParsedTemplateInfo &TemplateInfo, ...) {
...
  bool shouldDelayDiagsInTag =
    (TemplateInfo.Kind == ParsedTemplateInfo::ExplicitInstantiation ||
     TemplateInfo.Kind == ParsedTemplateInfo::ExplicitSpecialization);
  SuppressAccessChecks diagsFromTag(*this, shouldDelayDiagsInTag);
...
}

```
> And then you've got this other piece, a parser hack,
Totally agree. It also looks like a hack for me. The point is that the principle of parser's workflow is that it analyzes tokens and then consume them one by one at the same time. So we are not confident whether we are parsing a primary template or specialization. Thus `TemplateInfo.Kind` is not always correct. To define a specialization we should look forward at the whole declaraion without consuming any tokens to leave them for further parsing but set a correct 'Kind'.
On the other side we can somehow store the context of we are parsing a template declaration and throw it through a bunch of calls. But I found this way much more complicated, than of looking at some next tokens. BTW @rsmith suggested this approach in the comment https://reviews.llvm.org/D78404#inline-717620
> Btw, I'm not an approver on this (and shouldn't be); if you want better review you might want to ping someone who's touched this code lately (according to git blame).
Thank you. I've already added all related people in the list of reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92024