From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-40133.protonmail.ch (mail-40133.protonmail.ch [185.70.40.133]) by sourceware.org (Postfix) with ESMTPS id 9EC543858D33 for ; Mon, 27 Nov 2023 01:45:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9EC543858D33 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 9EC543858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=185.70.40.133 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701049505; cv=none; b=X+JXRy81SO402xDAGxDiE3cxE5d/j7W6is0ukOAWjY3pDMSW4EWBUX1LZqkPc5HGfeZXgxo6vbMG9amt7WlTng28nttkna7XJz6WX9dVl/dZRxqvfr62w7829CHsCc4Tzu4Z3buYyS0tJM8u92xXnMBS7LshQElZEQBaHr/eU6I= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701049505; c=relaxed/simple; bh=GNhJsZneAJO2zP6A7SLne6WYbuSrMhxw6vaYnpJOUzQ=; h=DKIM-Signature:Date:To:From:Subject:Message-ID:MIME-Version; b=kcLy5Rs5gNnO5Lm9lowJ2maOgO7vvHTtcOF8A9RJDcpNxEgtxKVRzNZNaIZNXvareHKjK2MVgRaRoySDXdLivrdkPMnJvX9knRG4R+7mYPXbh2B5oKJGkPtN50yP7yaVghdD9t2oryMiT8nXdP4I+EPjKA4Y9MMoBT61UN4hzSk= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1701049501; x=1701308701; bh=GNhJsZneAJO2zP6A7SLne6WYbuSrMhxw6vaYnpJOUzQ=; 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=IQzEWKrAD4CqIGXbS00jZ0eA/SHcRZ6wm1X4oJMt5if6xpMWDwuN/84ptL1fu0EKQ BEz4e3tHCFbZW0upbQVL1UNecn1Z4jPBelbpjn9grc5dXF6+VhL2wQNP5qqFkZWxEV w+3/LmC1i+vcrz7q41qDd+yCoQd3oFBT35LbKhucRFvkBSSjxpNx4kxWRQS1lkLpjV FWzoy3Q+HU873dnByYKXB2gEy5M1wvcniFe5TdPjxK6+kvxPWCYKs7IyQWt3yTx13q d9sdgqede/ayfUyXEmULXtaaeLLU+c2T5fycMB5iav+GqJzvA1+CxrKYlnlDUSkoPO kCZqPFnkfj02Q== Date: Mon, 27 Nov 2023 01:44:35 +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: In-Reply-To: <7623e2db-ba29-42f2-85df-c2e796d7305b@redhat.com> References: <9OlK_2c3Punfm3w-wEHqyHGyKGG8Gr_K0BUnDOuC9Aazur4mWlAM5WuL1Ea0AMLvFLl6LKFVTs813yY0zA7m0_ji_R9qhE52G7MZXrVPfZE=@protonmail.com> <7Xr5Vil7ptZzPaCtc_ZCdcTPuUVY7dheOnklF-vVDb5_jl8PivYWgTT_f3cKLvg7IMnDruCDDrICRI6WVrUT3f8ZScGKAh4ATIkYSuRqPZc=@protonmail.com> <-SP7aKgN1FZED-RAPr2FBDuCrcwnu9-UhHcRXNEsNZRwIzJXCdhVbtBP921Yn8g71d0WL7XpFRetUlBAStzRpZB8p4yj5moRS0DIE9D6cnY=@protonmail.com> <7623e2db-ba29-42f2-85df-c2e796d7305b@redhat.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.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,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: > > > > OKAY, I figured out SOMETHING, I think this should be fine. As note= d in > > > > the comments, this might be a better way of handling the static lam= bda > > > > 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, mutatin= g > > > > things this way makes me uneasy though. > > >=20 > > > A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is t= hat > > > all equivalent FUNCTION_TYPE share the same tree node (through > > > type_hash_canon), so you're messing with the type of unrelated functi= ons > > > at the same time. I think it's better to stick with the way static > > > lambdas are handled. > >=20 > > 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? >=20 >=20 > 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. Oh... yeah..., I see the issue now. I still don't think the solution used for static lambdas will work, or be painless anyhow, but if I can't find something better I will try to use that one. > > 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. >=20 >=20 > 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. I'm still concerned it wont cooperate with xobj parameters of unrelated type, but like I said, you've indicated my solution is definitely wrong so I'll look at fixing it. > > 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. > >=20 > > 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. > >=20 > > template > > struct S : T { > > using T::operator(); > > void operator()(this int&, auto) {} > > }; > >=20 > > int main() > > { > > auto s0 =3D S{[](this auto&&, auto){}}; > > // error, ambiguous > > void (*p0)(int&, int) =3D &decltype(s0)::operator(); > >=20 > > auto s1 =3D S{[x =3D 42](this auto&&, auto){}}; > > // SFINAE's, calls S::operator() > > void (*p1)(int&, int) =3D &decltype(s1)::operator(); > > } > >=20 > > 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. >=20 >=20 > Agreed. Perfect, glad we are on the same page there. > > > > 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 depe= nds > > > > on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-con= st > > > > here. > > > > auto f1 =3D [n =3D 5](this auto&& self){ n =3D 10; }; > > > > as_const(f1)(); > > >=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. > >=20 > > 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. >=20 >=20 > Ah, yes, I see it now. I don't remember if I relayed my planned fix for this to you. My current idea is to modify the tree during instantiation of the lambda's body somewhere near tsubst and apply const to all it's members. This is unfortunately the best idea I have so far and it feels like an awful hack. I am open to better ideas, but I don't think we can do anything until the template is instantiated so I think it has to be there. > > > > 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 lit= tle > > > > 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 imple= ment > > > > the errors for unrelated type now. I'm still a little confused on w= hat > > > > 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 err= ors > > > > are supposed to be emitted when calling back into non-template stag= es > > > > of the compiler with substituted types. It also seems like that has= n't > > > > always been strictly followed, and I hate to contribute to code deb= t > > > > 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 > > > 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. > >=20 > > 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. >=20 >=20 > Makes sense, we won't see a message about the unrelated type because > print_candidates doesn't repeat deduction like print_z_candidates does. >=20 > > 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. >=20 >=20 > 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 >=20 > Jason Fair enough. Luckily, since I found that other test case, I have been able to confirm that my diagnostic is properly reported when print_z_candidates is called. So I feel like the diagnostic is implemented correctly. Should I wait until I fix the issue in tsubst_lambda_expr before submitting the patch? I'm fine to do it either way, just whatever you prefer. If I finish cleaning up these tests before I hear back I'll go ahead and submit it and then start looking at different solutions in there. Alex