From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-4316.protonmail.ch (mail-4316.protonmail.ch [185.70.43.16]) by sourceware.org (Postfix) with ESMTPS id D225F3858D1E for ; Sat, 11 Nov 2023 10:43:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D225F3858D1E 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 D225F3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=185.70.43.16 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699699424; cv=none; b=C64DNDxAhLsVeiU6agg/mrf/lIVaGP3mEZrFEkOBz1EDGc0IiMK+1H2g2Eo8dPj8TzTYhVx7ajFvOPTt12sHMT6mi8arTnxBpb3a3LDNZzqWNbz0eYV1Jo6LlrgZOx97//uNeRFzBHc/GqbkCqlxqhvqvN4UGZa7oqIuEfWbMSc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699699424; c=relaxed/simple; bh=0yjYEszDc0cIZfTH5mYkVUMHv33MuI/d1uGkBNna2dQ=; h=DKIM-Signature:Date:To:From:Subject:Message-ID:MIME-Version; b=erZh2mg5Y+YAGP+kkoWtFP2SxNk2ClHVe/qmJ0ulYxgaZkcq6xQ9pKWwUKObt7BweoklXV4j1aORTnWJz03rBzwDf3lCvamesl6i4Z9bx/sXo6MSlkfTiNKUIwW5g15u1LuJhw51791+uHekZQ6RBhnlls8W1J6Pm5djWSksQF4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1699699419; x=1699958619; bh=tEDTFyFYyYhPyAvrgMdWpLXvLfvXWrmVJ4x2gT03TM4=; 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=YmGKSV0TS4zPWrLV6PBKUEZp5tgbnHjpIFUnPXeKFG/GR3QtuiDxVPxhL51J+GIpu TW+eovRCaLNRItlE4xDfXuyvEYR7ccxkfg4qtsTEP9Jro62nTSPjEzP6G2vbAfOWk4 c8oYPWxDU9Hw/rZhJifd8oZgI+4S9Irx541IhV7kYXD/U62nbC+HTinc4XelefIIAH tDQvarJepaEBeDzBfXjRC1T07iRQoTzmXdRbk6hwbOLEG0S2udv691rOKi12iJubZ9 RCPFkHiVteH1TPIfeLmZxPmHAgkADjbPy0DETXz6tJ0iZy+tqHSkgvE12iNU6374UL ECO6FpSsXhf9Q== Date: Sat, 11 Nov 2023 10:43:23 +0000 To: Jason Merrill From: waffl3x Cc: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609] Message-ID: In-Reply-To: References: <2f4020bf-3ef7-4627-9d92-c74676981f4b@redhat.com> <2nV4Jls2nmL2lahW6X-HZsBb90H1QZJhy3ApVC8xpxvP6KUYDriDYJpxzqjDnshOUEQ6ehYzuXU6rJmPvj3l6jtqAniFlH4Buyfls4E8cfY=@protonmail.com> <00094736-8325-4c83-9237-a6c15c324c24@redhat.com> <3tSXKK_P5IpLLm2VJ76q-eiLPZhiaC6_ZpwOrej22LWsmGA_YXipLXwLdLNeFlShJaqpFH_LPQW6tqkPD1WlFnExnL-iDoW9CoA-KQETWYw=@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=-2.8 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: > [combined reply to all three threads] >=20 > On 11/9/23 23:24, waffl3x wrote: >=20 > > > > I'm unfortunately going down a rabbit hole again. > > > >=20 > > > > --function.h:608 > > > > `/* If pointers to member functions use the least significant bit t= o indicate whether a function is virtual, ensure a pointer to this function= will have that bit clear. */ #define MINIMUM_METHOD_BOUNDARY \\\\ ((TARGET= _PTRMEMFUNC_VBIT_LOCATION =3D=3D ptrmemfunc_vbit_in_pfn) \\\\ ? MAX (FUNCTI= ON_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)` > > >=20 > > > So yes, it was for PMFs using the low bit of the pointer to indicate = a > > > virtual member function. Since an xob memfn can't be virtual, it's > > > correct for them to have the same alignment as a static memfn. > >=20 > > Is it worth considering whether we want to support virtual xobj member > > functions in the future? If that were the case would it be better if we > > aligned things a little differently here? Or might it be better if we > > wanted to support it as an extension to just effectively translate the > > declaration back to one that is a METHOD_TYPE? I imagine this would be > > the best solution for non-standard support of the syntax. We would > > simply have to forbid by-value and conversion semantics and on the > > user's side they would get consistent syntax. > >=20 > > However, this flies in the face of the defective/contradictory spec for > > virtual function overrides. So I'm not really sure whether we would > > want to do this. I just want to raise the question before we lock in > > the alignment, if pushing the patch locks it in that is, I'm not really > > sure if it needs to be stable or not. >=20 >=20 > It doesn't need to be stable; we can increase the alignment of decls as > needed in new code without breaking older code. Okay great, good to know, it seems so obvious when you put it that way. > > > > All tests seemed to pass when applied to GCC14, but the results did > > > > something funny where it said tests disappeared and new tests appea= red > > > > and passed. The ones that disappeared and the new ones that appeare= d > > > > looked like they were identical so I'm not worrying about it. Just > > > > mentioning it in case this is something I do need to look into. > > >=20 > > > That doesn't sound like a problem, but I'm curious about the specific > > > output you're seeing. > >=20 > > I've attached a few test result comparisons so you can take a look. >=20 >=20 > Looks like you're comparing results from different build directories and > the libitm test wrongly includes the build directory in the test "name". > So yeah, just noise. AH okay that makes sense. > > Side note, would you prefer I compile the lambda and by-value fixes > > into a new version of this patch? Or as a separate patch? Originally I > > had planned to put it in another patch, but I identified that the code > > I wrote in build_over_call was kind of fundamentally broken and it was > > almost merely coincidence that it worked at all. In light of this and > > your comments (which I've skimmed, I will respond directly below) I > > think I should just revise this patch with everything else. >=20 >=20 > Agreed. Will do then. > > > > There are a few known issues still present in this patch. Most impo= rtantly, > > > > the implicit object argument fails to convert when passed to by-val= ue xobj > > > > parameters. This occurs both for xobj parameters that match the arg= ument type > > > > and xobj parameters that are unrelated to the object type, but have= valid > > > > conversions available. This behavior can be observed in the > > > > explicit-obj-by-value[1-3].C tests. The implicit object argument ap= pears to be > > > > simply reinterpreted instead of any conversion applied. This is ela= borated on > > > > in the test cases. > > >=20 > > > Yes, that's because of: > > >=20 > > > > @@ -9949,7 +9951,8 @@ build_over_call (struct z_candidate cand, int= flags, tsubst_flags_t complain) > > > > } > > > > } > > > > / Bypass access control for 'this' parameter. */ > > > > - else if (TREE_CODE (TREE_TYPE (fn)) =3D=3D METHOD_TYPE) > > > > + else if (TREE_CODE (TREE_TYPE (fn)) =3D=3D METHOD_TYPE > > > > + || DECL_XOBJ_MEMBER_FUNC_P (fn)) > > >=20 > > > We don't want to take this path for xob fns. Instead I think we need = to > > > change the existing: > > >=20 > > > > gcc_assert (first_arg =3D=3D NULL_TREE); > > >=20 > > > to assert that if first_arg is non-null, we're dealing with an xob fn= , > > > and then go ahead and do the same conversion as the loop body on firs= t_arg. > > >=20 > > > > Despite this, calls where there is no valid conversion > > > > available are correctly rejected, which I find surprising. The > > > > explicit-obj-by-value4.C testcase demonstrates this odd but correct= behavior. > > >=20 > > > Yes, because checking for conversions is handled elsewhere. > >=20 > > Yeah, as I noted above I realized that just handling it the same way as > > iobj member functions is fundamentally broken. I was staring at it last > > night and eventually realized that I could just copy the loop body. I > > ended up asserting in the body handling the implicit object argument > > for xobj member functions that first_arg !=3D NULL_TREE, which I wasn't > > sure of, but it seems to work. >=20 >=20 > That sounds like it might cause trouble with >=20 > struct A { > void f(this A); > }; >=20 > int main() > { > (&A::f) (A()); > } I will check to see what the behavior with this is. This sounds related to the next question I asked as well. > > I tried asking in IRC if there are any circumstances where first_arg > > would be null for a non-static member function and I didn't get an > > answer. The code above seemed to indicate that it could be. It just > > looks like old code that is no longer valid and never got removed. > > Consequently this function has made it on my list of things to refactor > > :^). >=20 >=20 > Right, first_arg is only actually used for the implicit object argument, > it's just easier to store it separately from the arguments in (). I'm > not sure which code you mean is no longer valid? Yeah I agree that it's easier to store it separately. -- call.cc:build_over_call ``` else if (TREE_CODE (TREE_TYPE (fn)) =3D=3D METHOD_TYPE) { tree arg =3D build_this (first_arg !=3D NULL_TREE ? first_arg : (*args)[arg_index]); ``` The trouble is, the code (shown above) does not assume that this holds true. It handles the case where the implicit object argument was passed in with the rest of the arguments. As far as I've observed, it seems like it's always passed in through the first_arg member of cand, which is what I was referring to here. > > ended up asserting in the body handling the implicit object argument > > for xobj member functions that first_arg !=3D NULL_TREE, which I wasn't > > sure of, but it seems to work. Since it wasn't clear what I was referring to, here is the code that I wrote (copied from the loop really) handling the case. In case it isn't obvious, I didn't snip the code in the METHOD_TYPE block, it's just snipped here as it's not code I've modified. I'm hopeful that the case you mentioned above is not problematic, but like I said I will be sure to test it. -- call.cc:build_over_call ``` else if (TREE_CODE (TREE_TYPE (fn)) =3D=3D METHOD_TYPE) { /* SNIP */ } else if (DECL_XOBJ_MEMBER_FUNC_P (fn)) { gcc_assert (cand->first_arg); gcc_assert (cand->num_convs > 0); tree type =3D TREE_VALUE (parm); tree arg =3D cand->first_arg; bool conversion_warning =3D true; conv =3D convs[0]; /* Set user_conv_p on the argument conversions, so rvalue/base handli= ng knows not to allow any more UDCs. This needs to happen after we process cand->warnings. */ if (flags & LOOKUP_NO_CONVERSION) conv->user_conv_p =3D true; tsubst_flags_t arg_complain =3D complain; if (!conversion_warning) arg_complain &=3D ~tf_warning; if (arg_complain & tf_warning) maybe_warn_pessimizing_move (arg, type, /*return_p*/false); val =3D convert_like_with_context (conv, arg, fn, 0, arg_complain); val =3D convert_for_arg_passing (type, val, arg_complain); if (val =3D=3D error_mark_node) return error_mark_node; else argarray[j++] =3D val; /* Advance parameter chain, don't advance arg index */ parm =3D TREE_CHAIN (parm); // ++arg_index; ++i; is_method =3D 1; first_arg =3D NULL_TREE; } ``` Anyway, I hope we can assume that first_argument is populated for all non-static member function calls, and if we can't perhaps that is something we can change. The assumptions we can make are unclear with how the code handling METHOD_TYPE is right now. If we make it a guarantee that first_argument is populated for non-static member function calls that code can be vastly simplified. > > > > Other than this, lambdas are not yet supported, > > >=20 > > > The error I'm seeing with the lambda testcase is "explicit object mem= ber > > > function cannot have cv-qualifier". To avoid that, in > > > cp_parser_lambda_declarator_opt you need to set quals to > > > TYPE_UNQUALIFIED around where we do that for mutable lambdas. > >=20 > > Yeah, this ended up being the case as suspected, and it was exactly in > > cp_parser_lambda_declarator_opt that it needed to be done. There's an > > additional quirk in start_preparsed_function, which I already rambled > > about a little in a previous reply on this chain, that suddenly > > restored setting up the 'this' pointer. Previously, it was being > > skipped because ctype was not being set for xobj member functions. > > However, ctype not being set was causing the scope to not be set up > > correctly for lambdas. In truth I don't remember exactly how the > > problem was presenting but I do know how I fixed it. > >=20 > > `tree fntype =3D TREE_TYPE (decl1); if (TREE_CODE (fntype) =3D=3D METHO= D_TYPE) ctype =3D TYPE_METHOD_BASETYPE (fntype); else if (DECL_XOBJ_MEMBER_= FUNC_P (decl1)) ctype =3D DECL_CONTEXT (decl1);` > >=20 > > In hindsight though, I have to question if this is the correct way of > > dealing with it. As I mentioned previously, there's an additional quirk > > in start_preparsed_function where it sets up the 'this' param, or at > > least, it kind of looks like it? This is where my rambling was about. > >=20 > > `/* We don't need deal with 'this' or vtable for xobj member functions.= */ if (ctype && !doing_friend && !(DECL_STATIC_FUNCTION_P (decl1) || DECL_= XOBJ_MEMBER_FUNC_P (decl1)))` > >=20 > > My solution was to just not enter that block for xobj member functions, > > but I'm realizing now that ctype doesn't seem to be set for static > > member functions so setting ctype for xobj member functions might be > > incorrect. I'll need to investigate it closer, I recall seeing the > > second block above and thinking "it's checking to make sure decl1 is > > not a static member function before entering this block, so that means > > it must be set." It seems that was incorrect. > >=20 > > At bare minimum, I'll have to look at this again. >=20 >=20 > Yeah, that could use some refactoring. IMO ctype should be set iff we > want to push_nested_class, and the code for setting up 'this' should > check for METHOD_TYPE instead of referring to ctype. I'll put it on the todo list for after this patch is done. I'll note that it seemed like not doing push_nested_class seemed to work just fine for xobj member functions, and only caused problems once I was working on lambdas. For this patch, I think I will change the condition for setting up 'this' instead. On the other hand, are we sure that lambdas don't assume 'this' was setup when accessing their captures? I will investigate this briefly but now that I've thought of that, maybe I won't mess around with this after all. On the other other hand, I'm pretty sure captures still work for lambdas even though that block isn't being handled now... or maybe I haven't tested them. Actually, I think it doesn't matter if lambdas do use that or not for their captures, because handling lambdas with an xobj parameter correctly will still involve changing how lambdas access their captures. PR102609 has a reply providing a test case when calling a lambdas call operator where the first argument is a type unrelated to the lambda's closure type. This might actually be relevant because I can think of a case where one could conditionally (if constexpr) access captures based on the type of the xobj parameter. THEN AGAIN, I don't think it's possible to pass an unrelated implicit object parameter to a lambda's call operator. (Not without explicitly calling it by function pointer of course.) There are types that are derived from the lambda, but those are still related so I assume that captures should be able to be accessed in such a case. Man, that raises even more questions for myself. If we are supposed to implicitly use the explicit object parameter for accessing captures... ([PR102609] Ga=C5=A1per A=C5=BEman from comment #17) > When a lambda has captures, the explicit object parameter is used to get = at > them *silently*, if they are related, otherwise the program is ill-formed= : ...then what happens when the type of the explicit object parameter is a class derived from the lambda, and the derived class has a member variable with the same name as a capture we are trying to access? I'm going to sit on this for a while and wait for feedback, I'm not planning on working on this section today anyway. > > > > I had wanted to write about some of my frustrations with trying to > > > > write a test for virtual specifiers and errors/warnings for > > > > shadowing/overloading virtual functions, but I am a bit too tired a= t > > > > the moment and I don't want to delay getting this up for another ni= ght. > > > > In short, the standard does not properly specify the criteria for > > > > overriding functions, which leaves a lot of ambiguity in how exactl= y we > > > > should be handling these cases. > > >=20 > > > Agreed, this issue came up in the C++ committee meeting today. See > > >=20 > > > https://cplusplus.github.io/CWG/issues/2553.html > > > https://cplusplus.github.io/CWG/issues/2554.html > > >=20 > > > for draft changes to clarify some of these issues. > >=20 > > Ah I guess I should have read all your responses before sending any > > back instead of going one by one. I ended up writing about this again I > > think. > >=20 > > struct B { > > virtual void f() const {} > > }; > >=20 > > struct S0 : B { > > void f() {} > > }; > >=20 > > struct S1 : B { > > void f(this S1&) {} > > }; > >=20 > > I had a bit of a debate with a peer, I initially disagreed with the > > standard resolution because it disallows S1's declaration of f, while > > S0's is an overload. I won't bore you with all the details of going > > back and forth about the proposed wording, my feelings are mostly that > > being able to overload something with an iobj member function but not > > being able to with an xobj member function was inconsistent. He argued > > that keeping restrictions high at first and lowering them later is > > better, and overloading virtual functions is already not something you > > should really ever do, so he was in favor of the proposed wording. >=20 >=20 > Right. As the author of this proposal said in discussion, "We want very > liberal overriding for explicit object member functions so that it'll > blow up." Yeah okay that makes sense, that was definitely what it felt like my friend was arguing, and I've come to agree with it. > > In light of our debate, my stance is that we should implement things as > > per the proposed wording. > >=20 > > struct B { > > virtual void foo() const&; > > }; > >=20 > > struct D : B { > > void foo(this int); > > }; > >=20 > > This would be ill-formed now according to the change in wording. My > > laymans interpretation of the semantics are that, the parameter list is > > the same when the xobj parameter is ignore, so it overrides. And since > > it overrides, and xobj member functions are not allowed to override, it > > is an error. >=20 >=20 > Yes. >=20 > > To be honest, I still don't understand where the wording accounts for > > the qualifiers of B::f, but my friend assured me that it is. >=20 >=20 > The qualifiers of B::f are part of the object parameter which we're > ignoring because the derived function is xobj. OKAY I get it now. Correspondence of the declarations involves the type of the object parameter, while whether something overrides or not doesn't take the object parameter into account, and it only takes the ref qualifiers of the function into account for iobj member functions. While for xobj member functions it ignores ref qualifiers. Alright, got it. > > Somewhat related is some warnings I wanted to implement for impossible > > to call by-value xobj member functions. Ones that involve an unrelated > > type get dicey because people could in theory have legitimate use cases > > for that, P0847R7 includes an example of this combining recursive > > lambdas and the overload pattern to create a visitor. However, I think > > it would be reasonable to warn when a by-value xobj member function can > > not be called due to the presence of overloads that take references. > > Off the top of my head I don't recall how many overloads it would take > > to prevent a by-value version from being called, nor have I looked into > > how to implement this. >=20 >=20 > Hmm, is it possible to make it un-callable? A function taking A and a > function taking A&& are ambiguous when called with an rvalue argument, > the by-value overload isn't hidden. Similarly, A and A& are ambiguous > when called with a cv-unqualified lvalue. I believe so, consider the following: struct S { void f(this S) {} void f(this S const&) {} void f(this S&&) {} }; I'm not 100% what the rules on this are, if I were to hazard a guess I bet that the reference overloads are taken because they have one less conversion? To be clear, I recognize that you could still select the by-value candidate by assigning it to a pointer to function type. I just don't really think this is a useful or typical case, while accidentally preventing a by-value xobj member function from being callable seems like something that could easily happen, especially if we had the following. struct S { template void f(this Self&&) {} /* Our class is small enough, optimize const calls here. */ void f(this S) {} } I could see this being a typical enough refactor that I would want a warning to alert me that my function that I'm writing for optimization purposes won't ever be called in the cases I intend it to be called in. Granted, I fully admit that I don't fully understand the semantics with overload resolution here so my concerns might be entirely unfounded. This is especially true in the second case since I'm not sure if the deduced case actually will eat up all the calls of the member function or not. > > But I do know that redeclarations of xobj member > > functions as iobj member functions (and vice-versa) are not properly > > identified when ref qualifiers are omitted from the corresponding (is > > this the correct term here?) iobj member function. >=20 >=20 > That would be the code in add_method discussed later in that message. Yeah, which I should get to before we hit our deadline. > > > > + /* If */ > > >=20 > > > I think this comment doesn't add much. :) > >=20 > > I wish I remembered what I even meant to put here, oh well, must not > > have been all that important. Oh wait, I remember it now, I was going > > to note that the next element in the chain of declarators being null > > seems to indicate that the declaration is a function type. I will > > probably put that comment in again, it's on the line of commenting > > exactly what the code does but it was cryptic to figure this out, I'm > > not sure if I should lean towards including the comment here or not. >=20 >=20 > I always lean towards including comments. :) I personally prefer to write code that doesn't need comments, but in a code base like this we don't really have that luxury. It would take massive amounts of refactoring to communicate assumptions through code instead of through comments. While I would like to move towards this, for now I will also lean towards including comments, and in this particular case I'll make sure to include what I wrote in the comment. > > > > + /* I can imagine doing a fixit here, suggesting replacing > > > > + this / *this / this-> with &name / name / "name." but it would be > > > > + very difficult to get it perfect and I've been advised against > > > > + making imperfect fixits. > > > > + Perhaps it would be as simple as the replacements listed, > > > > + even if one is move'ing/forward'ing, if the replacement is just > > > > + done in the same place, it will be exactly what the user wants? > > > > + Even if this is true though, there's still a problem of getting t= he > > > > + context of the expression to find which tokens to replace. > > > > + I would really like for this to be possible though. > > > > + I will decide whether or not to persue this after review. */ > > >=20 > > > You could pass the location of 'this' from the parser to > > > finish_this_expr and always replace it with (&name)? > >=20 > > I've been reluctant to make any changes to parameters, but if you're > > suggesting it I will consider it. Just replacing it with '(&name)' > > seems really mediocre to me though. Yeah, sure, it's always going to be > > syntactically valid but I worry that it wont be semantically correct > > often enough. > >=20 > > Yeah, considering that I think I will leave it for now until I have > > time to come up with a robust solution. I might still pass in the > > location of the this token though and use error_at instead of error. > > I'll see how I feel once I finish higher priority stuff. >=20 >=20 > Makes sense. >=20 > > To be clear, I should be replacing { dg-options "-Wno-error=3Dpedantic"= } > > with { dg-options "" } and you would prefer I just remove { dg-options > > "-pedantic-errors" } yeah? >=20 >=20 > Yes. Sure thing then. I'll try to get a new version of the patch out ASAP, I'm not sure I'll include everything we talked about right away but I'll stuff in as much as I can. I'll focus on the missing semantics before the more complicated diagnostics changes. I think detection of redeclarations, the correct overriding behavior, and the additional warnings I have been discussing, will come later. I think I'm also going to leave investigation of requires expressions in non-dependent contexts that I had mentioned for another time as well. The tests related to those have been fixed and work as expected right now, and I think diving in and trying to figure out whether I can do those tests with non-dependent operands would be a bad use of my time. Of course I will make sure all the small changes you requested make it in, that might be the first thing I work on. Alex