From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-4322.protonmail.ch (mail-4322.protonmail.ch [185.70.43.22]) by sourceware.org (Postfix) with ESMTPS id D9CE93858CD1 for ; Sun, 10 Dec 2023 15:23:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D9CE93858CD1 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 D9CE93858CD1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=185.70.43.22 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702221785; cv=none; b=Sm4Ubgk3vUVH0t+BW22VRPzIoJVTlqbC1GtANoH9wJZa6/xrjsVNtF91XXWjUxI6xObn3EnNlORqH2oHZHve2fdhoGBNj9pfMbG/fd4uY+qMATI3iF0tDpjAqXPtGXLMk1brnE59Jiwmfvk+NVDXmYZGP4EsyEwvwGGUo6XLltE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702221785; c=relaxed/simple; bh=8Be0S3XMRlJcAwPA2s/X3OZMv+2PWvZE0iA5vgrw3Yg=; h=DKIM-Signature:Date:To:From:Subject:Message-ID:MIME-Version; b=RKh3bD/1CPih6Tqp5AybJpVOjGYOoqlO2t0FjJoevD9oQaBVKy8LcbKgGP+AC5gO4PvPb5D6W9H4m+PO57Uzn3sBnQJr0l7FY1lm8G1Ro3JCewRhyE0WZVV9SMCVaJEpJpoiLNST8FpccZ+87+M/fkZklprZ6etb8+PFEnfP8u4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1702221780; x=1702480980; bh=Z3PpC6rocI/2a2meeYY41H5AVTzvD+bDXCy9HhBwkXQ=; 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=RAwIX9DFFBUtlcZWkBK2IghMnodLtd0GcJ8qcXHKexMuUQOLmUuQT62ZddjF25ALa 3Q37gof2BoS4gt6q5SGPrcluV44ge1jkdlTprYSUJHEVA8V9BeFjsvBPzognJHW9To 8abbxgigrnh8nJ6xKamj6AWV+x28ucjvlJ4uFaEe6pFC1WKXcX8cDVO++Op1HUbS13 FJQfwZ/suLaWCFjJVG1Q2oIyBewsfq4nVvrcmf60ChqseNOnzadu5M7R0Py/SsWsNv y1Vxw3qp+ITVVR8IOK5X9JOZ6fLH5Btt/2qMCnaYU75Ekh2C//coImCz3QN7kx8q85 0YD3AugfDHyQQ== Date: Sun, 10 Dec 2023 15:22:51 +0000 To: Jason Merrill From: waffl3x Cc: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH v7 1/1] c++: Initial support for P0847R7 (Deducing This) [PR102609] Message-ID: In-Reply-To: References: <_e1O52EjoN_BFiH31iHE-0eYegNJhoOdDN2O0mduqtMmt7qTGpRWgduxNppnO1si01rORJ470oWcoM-_lk1ICFo9lhe_ylBKQsJ791qMm_k=@protonmail.com> <1b3b0259-5ce4-4193-a36d-60f09e1c7c92@redhat.com> <575c0bbe-a3d6-4a54-b299-edff64df84b1@redhat.com> <59LofhYhxl7MLEuElXwZcESRB6MpjdG-iq-89B63siDRo5k0j-y6z-PVa6Y3iE1xE5LkJwpwTFi82bd0RZjB1yZbSJptFdPTBWfvOGj1W78=@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.5 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 Friday, December 8th, 2023 at 12:25 PM, Jason Merrill = wrote: >=20 >=20 > On 12/6/23 02:33, waffl3x wrote: >=20 > > Here is the next version, it feels very close to finished. As before, I > > haven't ran a bootstrap or the full testsuite yet but I did run the > > explicit-obj tests which completed as expected. > >=20 > > There's a few test cases that still need to be written but more tests > > can always be added. The behavior added by CWG2789 works in at least > > one case, but I have not added tests for it yet. The test cases for > > dependent lambda expressions need to be fleshed out more, but a few > > temporary ones are included to demonstrate that they do work and that > > the crash is fixed. Explicit object conversion functions work, but I > > need to add fleshed out tests for them, explicit-obj-basic5.C has that > > test. >=20 > > @@ -6586,6 +6586,17 @@ add_candidates (tree fns, tree first_arg, const = vec args, > > + / FIXME: I believe this will be bugged for xobj member functions, > > + leaving this comment here to make sure we look into it > > + at some point. > > + Seeing this makes me want correspondence checking to be unified > > + in one place though, not sure if this one needs to be different > > + from other ones though. > > + This function is only used here, but maybe we can use it in add > > + method and move some of the logic out of there? >=20 >=20 > fns_correspond absolutely needs updating to handle xob fns, and doing > that by unifying it with add_method's calculation would be good. >=20 > > + Side note: CWG2586 might be relevant for this area in > > + particular, perhaps we wait to see if it gets accepted first? */ >=20 >=20 > 2586 was accepted last year. >=20 > > @@ -12574,17 +12601,25 @@ cand_parms_match (z_candidate *c1, z_candidat= e c2) > > fn1 =3D DECL_TEMPLATE_RESULT (t1); > > fn2 =3D DECL_TEMPLATE_RESULT (t2); > > } > > + / The changes I made here might be stuff I was told not to worry abou= t? > > + I'm not really sure so I'm going to leave it in. */ >=20 >=20 > Good choice, this comment can go. >=20 > > tree parms1 =3D TYPE_ARG_TYPES (TREE_TYPE (fn1)); > > tree parms2 =3D TYPE_ARG_TYPES (TREE_TYPE (fn2)); > > if (DECL_FUNCTION_MEMBER_P (fn1) > > && DECL_FUNCTION_MEMBER_P (fn2) > > - && (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn1) > > - !=3D DECL_NONSTATIC_MEMBER_FUNCTION_P (fn2))) > > + && (DECL_STATIC_FUNCTION_P (fn1) > > + !=3D DECL_STATIC_FUNCTION_P (fn2))) > > { > > /* Ignore 'this' when comparing the parameters of a static member > > function with those of a non-static one. */ > > - parms1 =3D skip_artificial_parms_for (fn1, parms1); > > - parms2 =3D skip_artificial_parms_for (fn2, parms2); > > + auto skip_parms =3D [](tree fn, tree parms){ > > + if (DECL_XOBJ_MEMBER_FUNCTION_P (fn)) > > + return TREE_CHAIN (parms); > > + else > > + return skip_artificial_parms_for (fn, parms); > > + }; > > + parms1 =3D skip_parms (fn1, parms1); > > + parms2 =3D skip_parms (fn2, parms2); > > } >=20 >=20 > https://cplusplus.github.io/CWG/issues/2789.html fixes the handling of > xobj fns here. I have a minor concern here. https://eel.is/c++draft/basic.scope.scope#3 Is it okay that CWG2789 has separate rules than the rules for declarations? As far as I know there's nothing else that describes how to handle the object parameters so it was my assumption that the rules here are also used for that. I know at least one person has disagreed with me about that so I'm not sure what is actually correct. template struct S { constexpr void f() {} // #f1 constexpr void f(this S&&) requires true {} // #f2 constexpr void g() requires true {} // #g1 constexpr void g(this S&&) {} // #g2 }; void test() { S<>{}.f(); // calls #? S<>{}.g(); // calls #? } But with the wording proposed by CWG2789, wouldn't this example would be a problem? If we follow the standard to the letter, constraints can't be applied here right? I wouldn't be surprised if I'm missing something but I figured I ought to raise it just in case. Obviously it should call #f2 and #g1 but I'm pretty sure the current wording only allows these calls to be ambiguous. > Your change does the right thing for comparing static and xobj, but > doesn't handle comparing iobj and xobj; I think we want to share > parameter comparison code with fns_correspond/add_method. Maybe > parms_correspond? Yeah, I'll see what I can put together. The only issue being what I noted above, I'm not sure the rules are actually the same. I think they should be, but my reading of things seems like it's not right now. For the time being I'm going to assume things should work the way I want them to, because I don't think the example I presented above should be ambiguous. > > @@ -8727,21 +8882,42 @@ resolve_address_of_overloaded_function (tree ta= rget_type, > > /* Good, exactly one match. Now, convert it to the correct type. */ > > fn =3D TREE_PURPOSE (matches); > >=20 > > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn) > > - && !(complain & tf_ptrmem_ok) && !flag_ms_extensions) > > + if (DECL_OBJECT_MEMBER_FUNCTION_P (fn) > > + && !(complain & tf_ptrmem_ok)) > > { > > - static int explained; > > - > > - if (!(complain & tf_error)) > > + /* For iobj member functions, if if -fms_extensions was passed in, th= is > > + is not an error, so we do nothing. It is still an error regardless > > + for xobj member functions though, as it is a new feature we > > + (hopefully) don't need to support the behavior. */ >=20 >=20 > Unfortunately, it seems that MSVC extended their weirdness to xobj fns, > so -fms-extensions should as well. > https://godbolt.org/z/nfvn64Kx5 Ugh, what a shame. We don't have to support it though do we? The documentation for -fms-extensions seems to state that it's for microsoft headers, if they aren't using xobj parameters in their headers as of now, and I can't see them doing so for the near future, it should be fine to leave this out still shouldn't it? The code will be simpler if we can't though so I reckon it's win/win whichever way we choose. > > + /* I'm keeping it more basic for now. */ >=20 >=20 > OK, this comment can go. Yeah, if I ever get comfortable with fixit's I'll do a loop through my changes and see where I can enhance the ones already present and add the ones that I wanted. > > @@ -15502,9 +15627,10 @@ void > > grok_special_member_properties (tree decl) > > { > > tree class_type; > > - > > + /* I believe we have to make some changes in here depending on the ou= tcome > > + of CWG2586. */ >=20 >=20 > As mentioned above, CWG2586 is resolved. Be sure to scroll down to the > approved resolution, or refer to the working draft. > https://cplusplus.github.io/CWG/issues/2586.html Yeah I got a bit of a lecture on how to read these in IRC, I shouldn't be making this mistake again. :'D I'll look at supporting it, I imagine it shouldn't be too hard but I've been wrong before. > > @@ -11754,8 +11754,16 @@ cp_parser_lambda_declarator_opt (cp_parser* pa= rser, tre > > else if (cxx_dialect < cxx23) > > omitted_parms_loc =3D cp_lexer_peek_token (parser->lexer)->location; > >=20 > > + /* Review note: I figured I might as well update the comments since I= 'm here. > > + There are also some additions to the below. */ >=20 >=20 > Great, this comment can go. Just noting, I'm also removing the old comment immediately following this one as it is obsolete. > > + /* [expr.prim.lambda.general-4] > > + If the lambda-declarator contains an explicit object parameter > > + ([dcl.fct]), then no lambda-specifier in the lambda-specifier-seq > > + shall be mutable or static. / > > + if (lambda_specs.storage_class =3D=3D sc_mutable) > > + { > > + auto_diagnostic_group d; > > + error_at (lambda_specs.locations[ds_storage_class], > > + "% lambda specifier " > > + "with explicit object parameter"); > > + / Tell the user how to do what they probably meant, maybe fixits > > + would be apropriate later? */ >=20 >=20 > "appropriate" >=20 > > + if (!dependent_type_p (non_reference (param_type))) > > + /* If we are given a non-dependent type we will have already given > > + a diagnosis that the following would contradict with. */; >=20 >=20 > Only if the lambda has captures, though? Oops, yeah I agree. > We could also change dependent_type_p to the more specific > WILDCARD_TYPE_P, I think, both here and just above. I don't understand the significance of this but I'll trust there is one. Better to be consistent with your other changes anyway. > > + else if (!TYPE_REF_P (param_type)) > > + inform (DECL_SOURCE_LOCATION (xobj_param), > > + "the passed in closure object will not be mutated because " > > + "it is taken by copy/move"); >=20 >=20 > "by value" >=20 > > @@ -3092,7 +3093,31 @@ finish_this_expr (void) > > return rvalue (result); > >=20 > > tree fn =3D current_nonlambda_function (); > > - if (fn && DECL_STATIC_FUNCTION_P (fn)) > > + if (fn && DECL_XOBJ_MEMBER_FUNCTION_P (fn)) > > + { > > + auto_diagnostic_group d; > > + error ("% is unavailable for explicit object member " > > + "functions"); > > + /* Doing a fixit here is possible, but hard, might be worthwhile > > + in the future. / > > + tree xobj_parm =3D DECL_ARGUMENTS (fn); > > + gcc_assert (xobj_parm); > > + tree parm_name =3D DECL_NAME (xobj_parm); > > + if (parm_name) > > + inform (DECL_SOURCE_LOCATION (xobj_parm), > > + "use explicit object parameter %qD instead", > > + parm_name); > > + else > > + { > > + gcc_rich_location xobj_loc (DECL_SOURCE_LOCATION (xobj_parm)); > > + / This doesn't work and I don't know why. I'll probably remove it > > + before the final version. */ > > + xobj_loc.add_fixit_insert_after (" self"); > > + inform (DECL_SOURCE_LOCATION (xobj_parm), > > + "name and use the explicit object parameter instead"); >=20 >=20 > Now seems to be the time to either fix or remove it. I think I'll defer it to future work, I'll pull it from the next version. Fixits are arcane, I'll have to do a deep dive on them to be able to use them properly I think. I'll also get in touch with David Malcolm as you suggested and see what I can learn about them from him. > > @@ -12811,9 +12836,11 @@ capture_decltype (tree decl) > > if (WILDCARD_TYPE_P (non_reference (obtype))) > > /* We don't know what the eventual obtype quals will be. / > > return NULL_TREE; > > - int quals =3D cp_type_quals (type); > > - if (INDIRECT_TYPE_P (obtype)) > > - quals |=3D cp_type_quals (TREE_TYPE (obtype)); > > + / This change possibly conflicts with another patch that is currently > > + in flight. (Patrick Palka's PR83167 patch) I am just changing it for > > + now to satisfy my tests. */ > > + int const quals =3D cp_type_quals (type) > > + | cp_type_quals (TREE_TYPE (obtype)); >=20 >=20 > Doesn't TREE_TYPE (obtype) break for by-value xob? In that case I think > we'd want cp_type_quals (obtype). I feel like you're right, and I'm disappointed that I missed this case in my tests. Indeed, I've checked and it ICEs for by-value xobj parameters. I'll fix this and add the test case. > > + The name "nonstatic" is no longer accurate with the addition of > > + xobj member functions, should we change the name? */ > >=20 > > bool > > invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t co= mplain) >=20 >=20 > This rule still applies to all non-static member functions: > https://eel.is/c++draft/expr.ref#6.3.2 Alright I see where my mistake is now, the comment regarding .* and ->* threw me for a loop. Even so, I figured since we renamed DECL_NONSTATIC_MEMBER_FUNCTION_P we might be better off renaming other things too. So in this case, invalid_object_memfn_p instead of invalid_nonstatic_memfn_p. Obviously changing the errors and warnings is a different beast and probably should be left alone. > > @@ -2352,7 +2355,7 @@ invalid_nonstatic_memfn_p (location_t loc, tree e= xpr, tsubst_flags_t complain) > > if (is_overloaded_fn (expr) && !really_overloaded_fn (expr)) > > expr =3D get_first_fn (expr); > > if (TREE_TYPE (expr) > > - && DECL_NONSTATIC_MEMBER_FUNCTION_P (expr)) > > + && DECL_IOBJ_MEMBER_FUNCTION_P (expr)) >=20 >=20 > ...and so I think this should be OBJECT. > https://godbolt.org/z/r6v4e1ePP Yeah, I agree, definitely a mistake on my part. Since we are here, following the same logic I presented above, can we refuse to support -fms-extensions for xobj member functions? Until microsoft is using xobj member functions in their headers we shouldn't need to support it right? Supporting their non-conformance going forward doesn't make sense to me. Especially since the non-conformance with xobj member functions is likely just because it was already this way. Until we know for sure that they rely on this behavior for xobj member functions I think it should remain off, it's a simple change to add it back in after all. > Here's a patch to adjust all the remaining > DECL_NONSTATIC_MEMBER_FUNCTION_P. With this patch -diagnostic7.C gets > the old address of non-static diagnostic, I think correctly, so I'm not > sure we still need the BASELINK change in cp_build_addr_expr_1? >=20 > Jason Thanks, I'll evaluate the BASELINK change to see if you're right. Hopefully you are, that would simplify things. Is there anything special I need to do when adding this patch? I was worried I'm supposed to maintain it's origin in it's own commit or something. Well, with that said it's probably still something I should learn to do anyway, I'm just trying really hard to put it off until the patch is done. Alex