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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
