On 12/15/25 10:32 PM, Patrick Palka wrote:
On Sun, 14 Dec 2025, Nathaniel Shead wrote:
On Sat, Dec 13, 2025 at 12:31:28PM -0500, Patrick Palka wrote:
On Sat, 13 Dec 2025, Patrick Palka wrote:
On Sat, 13 Dec 2025, Jason Merrill wrote:
On 12/13/25 7:50 PM, Nathaniel Shead wrote:
Bootstrapped and regtested on x86_64-pc-linux-gnu (so far just dg.exp
and modules.exp), OK for trunk if full regtest succeeds?
-- >8 --
When evaluating a concept definition in a template, any lambdas in the
definition of the concept get instantiated in the context of where the
evaluation occurred.
This causes two issues:
- Any lambdas declared later in the body of the function get the wrong
discriminator, which causes ABI divergences with Clang.
- Modules streaming gets confused, because the lambda is keyed to an
unrelated declaration. Keying the lambda to the concept also doesn't
work because we'd really want to key it to a concept instantiation
(that doesn't exist) so that merging works correctly.
I think really we just want to throw away these lambdas declarations
after evaluating the concept. They can (and will) be recreated in
importers re-evaluating the concept with the given args regardless.
This patch implements this by disabling scope recording for an
instantiation of a lambda keyed to a concept, and ensuring that the
lambda tag is added to an unrelated block that is then thrown away.
Would it make sense to just push_to(/pop_from)_top_level in
evaluate_concept_check? This seems like another instance of the recurring
problem of not pushing out of a local scope sufficiently before handling a
template.
This is related to PR104111. Some downsides of going this route:
template<class T> requires C<T> || D<T>
void f() {
if constexpr (C<T>) // potentially IFNDR if evaluation of C<T>
// depends on access context of f (though
// in practice we'll just reuse the cached
// value obtained earlier during satisfaction
// with the right access context)
...
else
...
}
--
template<class T> requires (!C<T>) // C<T> is not checked in
// access context of g
void g();
To me it seems that evaluating a concept-id in the access context
of where the concept-id appears is the better choice once we extend
the satisfaction cache to consider access context (which it currently
doesn't). Doing push_to_top_level would mean the above two testcases
could never work "as expected" even after we fix the satisfaction cache.
Oops, perhaps you didn't mean to just do push_to_top_level. If
we do push_to_top_level followed by push_access_scope to restore
the previous access context perhaps this wouldn't have an effect
on the PR104111 testcases.
Thanks for the comments. I think that makes sense to me; so something
like the following perhaps?
LGTM from my narrow PR104111 perspective. Jason what do you think?
I think this is a definite improvement.
I also think 104111 is probably wrong, CWG has been moving toward
clarifying that these evaluations should ignore local access context,
but it makes sense to address that separately rather than in this fix.
Jason
