From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-40134.protonmail.ch (mail-40134.protonmail.ch [185.70.40.134]) by sourceware.org (Postfix) with ESMTPS id 374E03858CD1 for ; Sun, 26 Nov 2023 23:11:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 374E03858CD1 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=protonmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=protonmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 374E03858CD1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=185.70.40.134 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701040281; cv=none; b=ItdxaUmtImUqzGH6RUB3NEFXR83KIkuBJBA+/bTDJX3MUlAkE094/Ew4ut9EUVY91ohpe9SwJjlUOTMCcyO9qE/qEXaVX6gd01/bfQHspjhpkWD+lBU7vnEprRhF+by9J3yMjQFAPCUB2sKEwAw3jdP9TA0bA1POZvPVP+chJto= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701040281; c=relaxed/simple; bh=lnUmns8von/9SfAVlBjM0SYrF3pPQ904QJAhHo4G8Kk=; h=DKIM-Signature:Date:To:From:Subject:Message-ID:MIME-Version; b=NhoDZUZPJFbHTxxjNeTgq89PYGSohmCoac06fd+vgjjsyLTSev2SENJENu7ek0gnX1D6XyN6NL+1PTT2WefGSECUKzpETkNdZS85aOqpOiFlfLfsL8IJ/YPj89b22+O06Edank22/RWiosdfsp8IpdUuSpQBMUSnt7pt+0Ccgpw= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1701040274; x=1701299474; bh=aWk5BIUcvWe5zgBHw1UpJT3tRo1zKXjGZEIcwfyWZgM=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=EIFQ2NjJFm4zHSaYI4ClCxyCuhQmg17EHw/Gi7KEfYQ+RyF9HOXBS/K/YNddRWLie N6IBtHv/Gmll07vKCjgpTzRpDcpRlVLMPIOnd0UEZs0rRaR5RNGm5TJWvdnOXmnu8K hBxuTg1VUt5ITcqqGcY/lYaYNtl33E4XgiFQmNHAK6wugptGubymeK3alM+uG6pOfU zj9z+zbYw17kA46YwgXycnqr8f/a0waHcHVm0YqYoSOJUc74r9QcZzyphZjlWYG0jJ 3lWYwlKL2nbDRDxV8X01rrJD3BoSxQHyotI7mG7IfOKVRKaqE2WW5MtIDVGSxIyinM Ow86J4puHa8MQ== Date: Sun, 26 Nov 2023 23:10:53 +0000 To: Jason Merrill From: waffl3x Cc: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH v5 1/1] c++: Initial support for P0847R7 (Deducing This) [PR102609] Message-ID: <-SP7aKgN1FZED-RAPr2FBDuCrcwnu9-UhHcRXNEsNZRwIzJXCdhVbtBP921Yn8g71d0WL7XpFRetUlBAStzRpZB8p4yj5moRS0DIE9D6cnY=@protonmail.com> In-Reply-To: References: <9OlK_2c3Punfm3w-wEHqyHGyKGG8Gr_K0BUnDOuC9Aazur4mWlAM5WuL1Ea0AMLvFLl6LKFVTs813yY0zA7m0_ji_R9qhE52G7MZXrVPfZE=@protonmail.com> <7Xr5Vil7ptZzPaCtc_ZCdcTPuUVY7dheOnklF-vVDb5_jl8PivYWgTT_f3cKLvg7IMnDruCDDrICRI6WVrUT3f8ZScGKAh4ATIkYSuRqPZc=@protonmail.com> Feedback-ID: 14591686:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Sunday, November 26th, 2023 at 2:30 PM, Jason Merrill = wrote: >=20 >=20 > On 11/24/23 20:14, waffl3x wrote: >=20 > > 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. >=20 >=20 > 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? 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. > > I don't like that pt.cc feels like it has a ton of hidden mutations, > > it's really hard to follow through it. Would you agree it's in need for > > cleanup or am I just not experienced enough in this area yet? >=20 >=20 > I'm sure there are things that could use cleaning up, but I'm not > thinking of anything specific offhand. Any particular examples? Not at the moment, just after 8 hours of following the execution path you start to feel like something could be done better. I'm sorry if I sound bitter but to be honest I kind of am. :^) I don't think it's going to be my first goal to look at pt.cc for things to clean up, I honestly didn't want to go through here at all. Maybe if I look at it for longer I will feel better about it but I want to make that a task for the future. > > 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. >=20 >=20 > 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. template struct S : T { using T::operator(); using s_tag =3D void; }; int main() { auto f =3D [](this auto self){ if constexpr ( requires{typename decltype(self)::s_tag;} ) { return 5; } else { return 10; } }; auto s =3D S{f}; if (f () !=3D 10) __builtin_abort (); if (s () !=3D 5) __builtin_abort (); } I hope this demonstrates that placing the check in tsubst_function_decl is the correct way to do this. 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...=20 auto f =3D [x =3D 42](this auto&&) -> int { return x; }; int (*fp)(int&) =3D &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. 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 struct S : T { using T::operator(); void operator()(this int&, auto) {} }; int main() { auto s0 =3D S{[](this auto&&, auto){}}; // error, ambiguous void (*p0)(int&, int) =3D &decltype(s0)::operator(); auto s1 =3D S{[x =3D 42](this auto&&, auto){}}; // SFINAE's, calls S::operator() void (*p1)(int&, int) =3D &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. Luckily for me, that is what my implementation already did. The big comment in tsubst_function_decl does not reflect this conclusion yet so I will have to revise it a little bit before I can submit v6, but once I do that I will follow up this e-mail with it. https://eel.is/c++draft/dcl.fct#15 https://eel.is/c++draft/expr.prim#lambda.closure-5 > > The other problem I'm having is > >=20 > > auto f0 =3D [n =3D 5, &m](this auto const&){ n =3D 10; }; > > This errors just fine, the lambda is unconditionally const so > > LAMBDA_EXPR_MUTABLE_P flag is set for the closure. > >=20 > > 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 =3D [n =3D 5](this auto&& self){ n =3D 10; }; > > as_const(f1)(); >=20 >=20 > That sounds correct, why would this be an error? >=20 > 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. Anyway, yes I agree that it's -supposed- to depend on the object parameter, but that isn't the behavior I'm observing right now. Perhaps you're right that it's not based on the LAMBDA_EXPR_MUTABLE_P flag, I mostly just assumed that was the case, but it does seem to be based on the closure object. Now that I'm clear of the previous problem today will be spent focusing on this bug. > > 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. >=20 >=20 > 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. 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. > > One side note before I close up here, I don't like when *_P macros are > > used to set flags. Is this something else we should clean up in the fut= ure? >=20 >=20 > I don't think so, a wholesale renaming would just confuse existing > developers. I don't think changing *_P macros to be immutable, and adding corresponding *_SET or *_FLAG macros (or functions) would be confusing. I think it's much more confusing to have the inconsistent behavior. In case it's not clear, I'm not proposing we change any of the names here. I disliked the *_P name scheme at first, but as I became familiar with it I think it's good, especially once I realized that it stands for predicate. Even at that point though I was pretty sure that convention should stick as it's not worth the confusion just because a single developer, let alone a new one (me), doesn't like it. And I like it now so I absolutely wouldn't propose making that change. But I do think they should be immutable, this has other benefits anyway since it will be more clear where we observe state, and where we mutate it. > > I'm beginning to > > wonder if an overhaul that gets rid of the macros from the public > > interface is a good idea. I'm reluctant to suggest that as I've really > > warmed up to the macros a lot. They are used in a consistent and easy > > to understand way which is highly unlike the bad uses of macros that > > I've seen before that really obscure what's actually going on. But they > > are still macros, so maybe moving away from them is the right call, > > especially since there has started to be a mix-up of macros and > > functions for the same purposes. I'm mildly of the opinion that only > > one style should be used (in the public interface) because mixing them > > causes confusion, it did for me anyway. Perhaps I should open a thread > > on the general mail list and see what others think, get some input > > before I decide which direction to go with it. To be clear, when I say > > getting rid of macros, I want to emphasize I mean only in the public > > interface, I don't see any value in getting rid of macros under the > > hood as the way the checking macros are implemented is already really > > good and works. It would only cause problems to try to move away from > > that. I think I'll probably start to mess around with this idea as soon > > as this patch is done. >=20 >=20 > There has been some movement from macros toward inline functions since > the switch to C++, but that also has complications with > const-correctness, declaration order, and bit-fields. It's probably > good to use inlines instead of larger macros when feasible, but the > accessors should probably stay macros. >=20 > Jason Yeah I've noticed the pains with const-correctness, it seems like a nightmare. However, thats one of the reasons I want to tackle it, I think fixing and propagating const correctness throughout the code base would be the biggest thing that could be done to improve it. I have really poor short term memory so once you start piling on mutations I have a hard time reasoning about code really quickly. I see what you mean regarding accessors, trying to access or mutate bitfields through functions might be way more trouble than it's worth, especially if we wanted to keep the `SOME_FIELD (some_node) =3D new_value` pattern. We would need to return proxy objects and those just don't always optimize properly in my (limited) experience. I plan on playing around with some solutions once this patch is done, I'll see what feels good, what feels bad, whether it's actually viable to change the accessors into functions, etc. One developer in IRC expressed that he wished that accessors were member functions, which I also have kind of wished before. I feel like it shouldn't be the first refactor though. I will follow up this e-mail shortly with v6, I just have to clean up that comment as I said. I don't want to spend too much time on it because I want to get a proof-of-concept fix for the "lambda capture's not const" bug today but I'll try to throw it together with a little more effort than v5. I do want to note though, despite my claims above that the patch is bootstrapped and tested without regressions, I did have to make a small change in tsubst_lambda_expr. When I refactored the changes there I made a thinko and checked for iobj member function in the condition before build_memfn_type, completely forgetting about the static lambda case. I changed it back to !DECL_XOBJ_MEMBER_FUNC_P and ran the 1 test that failed (static-operator-call5.C) and confirmed that it worked again, but running the full test suite takes me about an hour and a half. SO, point is, I'm confident that change broke nothing, but due to the change technically this version is not regression tested. I am running a bootstrap since that will finish before I'm done revising the comment though. Hope this isn't a problem. I will try to finish up quickly so you can get your eyes on the patch ASAP. Alex