On 11/26/23 18:10, waffl3x wrote:
On Sunday, November 26th, 2023 at 2:30 PM, Jason Merrill <ja...@redhat.com>
wrote:
On 11/24/23 20:14, waffl3x wrote:
OKAY, I figured out SOMETHING, I think this should be fine. As noted in
the comments, this might be a better way of handling the static lambda
case too. This is still a nasty hack so it should probably be done
differently, but I question if making a whole new fntype node in
tsubst_lambda_expr makes sense. On the other hand, maybe there will be
problems if a lambda is already instantiated? I'm not sure, mutating
things this way makes me uneasy though.
A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is that
all equivalent FUNCTION_TYPE share the same tree node (through
type_hash_canon), so you're messing with the type of unrelated functions
at the same time. I think it's better to stick with the way static
lambdas are handled.
Yes, that was my concern as well, without even thinking about hashing.
I will investigate it deeper but I'm pretty sure TYPE_METHOD_BASETYPE,
while a valid field, is not used at all for function_type nodes. I had
to look into this when looking into the DECL_CONTEXT stuff previously,
so I'm pretty confident in this. Come to think of it, does hashing even
take fields that aren't "supposed" to be set for a node into account?
It doesn't. The issue is messing with the type of (potentially) a lot
of different functions. Even if it doesn't actually break anything, it
seems like the kind of hidden mutation that you were objecting to.
Well, even so, I can just clear it after it gets used as we just need
it to pass the closure type down. Perhaps I should have led with this,
but as it stands the version that uses TYPE_METHOD_BASETYPE bootstraps
with no regressions. I'll still look deeper but I'm pretty confident in
my decision here, I really don't want to try to unravel what
build_memfn_type does, I would rather find a whole different way of
passing that information down.
But the existing code already works fine, it's just a question of
changing the conditions so we handle xob lambdas the same way as static.
Regarding the error handling, I just had a thought about it, I have a
hunch it definitely needs to go in tsubst_template_decl or
tsubst_function_decl. There might need to be more changes to determine
the actual type of the lambda in there, but everything else I've done
changes the implicit object argument to be treated more like a regular
argument, doing error handling for the object type in
tsubst_lambda_expr would be inconsistent with that.
But the rule about unrelated type is specific to lambdas, so doing it in
tsubst_lambda_expr seems preferable to adding a lambda special case to
generic code.
No, while that might seem intuitive, the error is not during
instantiation of the closure object's type, it's during instantiation
of the function template. The type of the closure will always be the
type of the closure, but the type of the explicit object parameter can
be different. In the following example we don't go into
tsubst_lambda_expr at all.
Ah, good point.
I hope this demonstrates that placing the check in tsubst_function_decl
is the correct way to do this.
Sounds right.
Now with that out of the way, I do still kind of feel icky about it.
This really is a MASSIVE edge case that will almost never matter. As
far as I know, instantiating explicitly like so...
auto f = [x = 42](this auto&&) -> int { return x; };
int (*fp)(int&) = &decltype(f)::operator();
...is the only way to coerce the explicit object parameter of the
lambda's call operator into deducing as an unrelated type. Cases that
are not deduced can be caught trivially while parsing the lambda and
are the only reasonable cases where you might have an unrelated type.
Perhaps it might become relevant in the future if a proposal like
https://atomgalaxy.github.io/isocpp-1107/D1107.html ever made it in,
but we aren't there yet. So as it stands, I'm pretty certain that it's
just impossible to instantiate a lambda's call operator with an
unrelated xobj parameter type except for the above case with
address-of. If you can think of another, please do share, but I've
spent a fair amount of time on it and came up with nothing.
I think you're right.
Anyway, due to this, I am not currently concerned about the lack of a
diagnostic, and I believe my implementation is the most correct way to
do it. The problem with resolve_address_of_overloaded_function that I
go into below (depending on if you agree that it's a problem) is what
needs to be fixed to allow the current diagnostic to be properly
emitted. As of right now, there is no execution path that I know of
where the diagnostic will be properly emitted.
I go into more detail about this in the comment in
tsubst_function_decl, although I do have to revise it a bit as I wrote
it before I had a discussion with another developer on the correct
behavior of the following case. I previously wondered if the overload
resolution from initializing p1 should result in a hard fault.
template<typename T>
struct S : T {
using T::operator();
void operator()(this int&, auto) {}
};
int main()
{
auto s0 = S{[](this auto&&, auto){}};
// error, ambiguous
void (*p0)(int&, int) = &decltype(s0)::operator();
auto s1 = S{[x = 42](this auto&&, auto){}};
// SFINAE's, calls S::operator()
void (*p1)(int&, int) = &decltype(s1)::operator();
}
The wording in [expr.prim.lambda.closure-5] is similar to that in
[dcl.fct-15], and we harness SFINAE in that case, so he concluded that
this case (initializing p1) should also harness SFINAE.
Agreed.
The other problem I'm having is
auto f0 = [n = 5, &m](this auto const&){ n = 10; };
This errors just fine, the lambda is unconditionally const so
LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
This on the other hand does not. The constness of the captures depends
on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
here.
auto f1 = [n = 5](this auto&& self){ n = 10; };
as_const(f1)();
That sounds correct, why would this be an error?
The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P,
it depends on the type of the object parameter, which in this case is
non-const, so so are the captures.
Oops, I should have just made a less terse example, you missed the
"as_const" in the call to the lambda. The object parameter should be
deduced as const here. I definitely should have made that example
better, my bad.
Ah, yes, I see it now.
I was going to close out this message there but I just realized why
exactly you think erroring in instantiate_body is too late, it's
because at that point an error is an error, while if we error a little
bit earlier during substitution it's not a hard error. I'm glad I
realized this now, because I am much more confident in how to implement
the errors for unrelated type now. I'm still a little confused on what
the exact protocol for emitting errors at this stage is, as there
aren't many explicit errors written in. It seems to me like the errors
are supposed to be emitted when calling back into non-template stages
of the compiler with substituted types. It also seems like that hasn't
always been strictly followed, and I hate to contribute to code debt
but I'm not sure how I would do it in this case, nor if I actually have
a valid understanding of how this all works.
Most errors that could occur in template substitution are guarded by if
(complain & tf_error) so that they aren't emitted during deduction
substitution. It's better if those are in code that's shared with the
non-template case, but sometimes that isn't feasible.
Yeah, makes sense, but the case going through
resolve_address_of_overloaded_function -> fn_type_unification ->
instantiate_template -> tsubst_decl -> tsubst_function_decl does not
behave super nicely. I tried adding (complain & tf_error) to the
explain_p parameter of fn_type_unification in
resolve_address_of_overloaded_function, but I immediately learned why
that wasn't being handled that way. I think
resolve_address_of_overloaded_function has a number of problems and
should probably use print_z_candidates instead of print_candidates in
the long term. This would actually solve the problem in
tsubst_function_decl where it never emits an error for an unrelated
type, print_z_candidates does call fn_type_unification with true passed
to explain_p. I go into this in detail in the large comment in
tsubst_function_decl (that I have to revise) so I won't waste time
rehashing it here.
Makes sense, we won't see a message about the unrelated type because
print_candidates doesn't repeat deduction like print_z_candidates does.
Thank you for explaining though, some of the code doesn't reflect the
method you've relayed here (such as an unguarded error_at in
tsubst_function_decl) so I was starting to get confused on what exactly
is supposed to be going on. This clarifies things for me.
That unguarded error does indeed look suspicious, though it could be
that OMP reductions are called in a sufficiently unusual way that it
isn't actually a problem in practice. /shrug
Jason
