[PATCH] D140876: [clang][C++20] Non-dependent access checks in requires expression.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 marked an inline comment as done.
usaxena95 added a comment.

In D140876#4023286 <https://reviews.llvm.org/D140876#4023286>, @ilya-biryukov wrote:

> Should access checks should happen in the context where `concept` is written or where it's used?
> If access-checking should happen where concept is defined, having a hard error seems fine because of the wording you quoted:

As Roy indicated, this is a wording issue. For the time being, concepts are not evaluated in the scope of its use (as the result of atomic constraints should be same at all points of program). An independent requires clause on the other hand considers the scope where it is written.

>> If the substitution of template arguments into a requirement would always result in a substitution failure, the program is ill-formed; no diagnostic required.
>
> The program is ill-formed and we show the diagnostic even though it's not required.

Are you suggesting to drop the diagnostic in this case ? I think

> I poked around and found an interesting case where GCC seems to be doing the wrong thing:
>
>   template <class> struct B;
>   class A {
>      static void f();
>      friend struct B<short>;
>   };
>    
>   template <class T> struct B {
>       static constexpr int index() requires requires{ A::f(); } {
>           return 1;
>       }
>       static constexpr int index() {
>           return 2;
>       }
>   };
>   
>   static_assert(B<short>::index() == 1); // GCC picks 1, MSVC picks 1.
>   static_assert(B<int>::index() == 2);   // GCC (incorrectly?) picks 1, MSVC picks 2!
>
> Is this related to this change? Could we add a test that validates Clang is doing the right thing?

This is an interesting test case. Clang does the right thing here and it is related to both this change and the base change https://reviews.llvm.org/D140547.
The test case mentioned on that patch also needs both the changes and also something else.

I will squash both these patches into one for a better review/commit.



================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3510
       Actions, Sema::ExpressionEvaluationContext::Unevaluated);
+  Sema::ContextRAII SaveContext(Actions, Actions.CurContext);
 
----------------
ilya-biryukov wrote:
> Could you explain how this changes behaviour and how it leads to fixing the issue?
> 
> I'm sure there is quite a bit of thought and debugging behind this one-line change, but it's not evident by just looking at it how it solves the issue.
Sorry for not providing more context here. I agree this makes things work quite unexpectedly. 

The main issue here is the delaying of the diagnostics. Eg in `Parser::ParseTemplateDeclarationOrSpecialization` through `ParsingDeclRAIIObject ParsingTemplateParams`. Diagnostics in `ParseConceptDefinition` are attached to  Diagnostics pool of `ParsingTemplateParams` which are never flushed. Creating a new context does not delay the diagnostics. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140876