From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id 30164385C41B for ; Wed, 22 Nov 2023 20:47:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 30164385C41B Authentication-Results: sourceware.org; dmarc=fail (p=quarantine dis=none) header.from=protonmail.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=protonmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 30164385C41B Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:470:142:3::10 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700686069; cv=none; b=fps/338HtEOo/ihW25VbC+7qqJyVuhFhTKut5O+swM2mcCdc2uKAq9R+O8q+61rsu1AdMbA7ayAzT8oZue7sb7zNByWy1g8h1bpEF+yaEdkHPjw/HUYtfl33DUSzvxw0CvxmJVm8L9JLle6E/ZeCd4LzB90KzP7/I36eWogYm7I= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700686069; c=relaxed/simple; bh=nx60HWwaE/GTPB7qWgGjQadfeh2TnLbFZNUOJz/mwQ0=; h=DKIM-Signature:Date:To:From:Subject:Message-ID:MIME-Version; b=QI5Vd0HrD7I04QRfpO25x7+C4m7dmGaViGMCIB829Nwho9i5jmNz7iEMNdvwRSJbuw6eXzVQSiEaOFXl7Ujf5sBLKY/fUrGKQFJBA2xiwfC2FEuimpTxkJwFcUuuzgBBdHyJ+2AYhRDyN1ZRQHjP+4aV0KvbsfOMD4GOuJRTA/I= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from mail-40133.protonmail.ch ([185.70.40.133]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r5u8F-0005cH-S7 for gcc-patches@gcc.gnu.org; Wed, 22 Nov 2023 15:47:38 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1700686021; x=1700945221; bh=lr7Qd+7agqzXAxPxPaz0nyZ2tSp3EJ34DyBNeUF/lTQ=; 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=n6xZ4ENN3ZX3OugqBYs+KoSQZEUUTE7NiwyNK/aPzkY5TDeERg30KdM5/GBwJcCV6 U7wCOHUJbyGl8e/mHgrfPUc0JBAOCdnPsLSGynXK/ZKZOENkcl3qfJxYQ09bDNyf8p d0UI1nrzq/bhyarAkLldJ4ucXdmZyGLG0IeeYOER/iVIIrUqtnAozWwBnbGsDlPFZb OFcziwkDISI0L7UO+Q2Ynr7L0dhadnxHDq6T0OGUrwgdzJKEuyjD0MYyCm4gd1wrlY wtR7TOyhy6ulqiytJNEevkdvpYo8J+UMlCa4NwtWLke/0tB9B3C0BERHC5mVpXejoI JfvcMB/3EP6Ow== Date: Wed, 22 Nov 2023 20:46:46 +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: <9OlK_2c3Punfm3w-wEHqyHGyKGG8Gr_K0BUnDOuC9Aazur4mWlAM5WuL1Ea0AMLvFLl6LKFVTs813yY0zA7m0_ji_R9qhE52G7MZXrVPfZE=@protonmail.com> In-Reply-To: References: <9890a007-755e-41e2-bc33-37ebf0755435@redhat.com> <1MdaTybBd_o4uw-Gb23fYyd5GNz7qFqoSe_f5h90LY_BdzM2ge2qPSyCuiCLYoYcZSjmVv13fw1LmjQC_M2L8raS1fydY5pEJ_vwvv5Z-0k=@protonmail.com> Feedback-ID: 14591686:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=185.70.40.133; envelope-from=waffl3x@protonmail.com; helo=mail-40133.protonmail.ch X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DKIM_VALID_EF=-0.1,FREEMAIL_FROM=0.001,RCVD_IN_MSPIKE_H5=0.001,RCVD_IN_MSPIKE_WL=0.001,SPF_HELO_PASS=-0.001,SPF_PASS=-0.001,T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,SPF_HELO_PASS,SPF_SOFTFAIL,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no 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 Tuesday, November 21st, 2023 at 8:22 PM, Jason Merrill wrote: > > > On 11/21/23 08:04, waffl3x wrote: > > > Bootstrapped and tested on x86_64-linux with no regressions. > > > > Hopefully this patch is legible enough for reviewing purposes, I've not > > been feeling the greatest so it was a task to get this finished. > > Tomorrow I will look at putting the diagnostics in > > start_preparsed_function and also fixing up anything else. > > > > To reiterate in case it wasn't abundantly clear by the barren changelog > > and commit message, this version is not intended as the final revision. > > > > Handling re-declarations was kind of nightmarish, so the comments in > > there are lengthy, but I am fairly certain I implemented them correctly= . > > > > I am going to get some sleep now, hopefully I will feel better tomorrow > > and be ready to polish off the patch. Thanks for the patience. > > > Great! > > > I stared at start_preparsed_function for a long while and couldn't > > figure out where to start off at. So for now the error handling is > > split up between instantiate_body and cp_parser_lambda_declarator_opt. > > The latter is super not correct but I've been stuck on this for a long > > time now though so I wanted to actually get something that works and > > then try to make it better. > > > I see what you mean, instantiate body isn't prepared for > start_preparsed_function to fail. It's ok to handle this in two places. > Though actually, instantiate_body is too late for it to fail; I think > for the template case it should fail in tsubst_lambda_expr, before we > even start to consider the body. > > Incidentally, I notice this code in tsubst_function_decl seems to need > adjusting for xobj: > > tree parms =3D DECL_ARGUMENTS (t); > if (closure && !DECL_STATIC_FUNCTION_P (t)) > parms =3D DECL_CHAIN (parms); > parms =3D tsubst (parms, args, complain, t); > for (tree parm =3D parms; parm; parm =3D DECL_CHAIN (parm)) > DECL_CONTEXT (parm) =3D r; > if (closure && !DECL_STATIC_FUNCTION_P (t)) > ... > > and this in tsubst_lambda_expr that assumes iobj: > > /* Fix the type of 'this'. */ > fntype =3D build_memfn_type (fntype, type, > type_memfn_quals (fntype), > type_memfn_rqual (fntype)); Originally I was going to say this doesn't look like a problem in tsubst_lambda_expr, but after looking at tsubst_function_decl I'm thinking it might be the source of some trouble. If it really was causing problems I would think it would be working much worse than it currently is, but it does feel like it might be the actual source of the bug I was chasing yesterday. Assigning to a capture with a deduced const xobj parameter is not being rejected right now, this might be causing it. I'll look more thoroughly today. > This also seems like the place to check for unrelated type. It does feel that way, I agree. > > /* Nonzero for FUNCTION_DECL means that this decl is a non-static > > - member function. */ > > + member function, use DECL_IOBJ_MEMBER_FUNC_P instead. */ > > #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \ > > (TREE_CODE (TREE_TYPE (NODE)) =3D=3D METHOD_TYPE) > > > > +/* Nonzero for FUNCTION_DECL means that this decl is an implicit objec= t > > + member function. */ > > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \ > > + (TREE_CODE (TREE_TYPE (NODE)) =3D=3D METHOD_TYPE) > > > I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than > add a new, equivalent one. And then go through all the current uses of > the old macro to decide whether they mean IOBJ or OBJECT. I figure it would be easier to make that transition if there's a clear line between old versus new. To be clear, my intention is for the old macro to be removed once all the uses of it are changed over to the new macro. I can still remove it for the patch if you like but having both and removing the old one later seems better to me. > > - (static or non-static). */ > > + (static or object). */ > > > Let's leave this comment as it was. Okay. > > + auto handle_arg =3D [fn, flags, complain](tree type, > > + tree arg, > > + int const param_index, > > + conversion *conv, > > + bool const conversion_warning) > > > Let's move the conversion_warning logic into the handle_arg lambda > rather than have it as a parameter. Yes, we don't need it for the xobj > parm, but I think it's cleaner to have less in the loop. I would argue that it's cleaner to have the lambda be concise, but I'll make this change. > Also, let's move handle_arg after the iobj 'this' handling so it's > closer to the uses. For which the 'else xobj' needs to drop the 'else', > or change to 'if (first_arg)'. Agreed, I didn't like how far away it was. > > + /* We currently handle for the case where first_arg is NULL_TREE > > + in the future this should be changed and the assert reactivated. */ > > + #if 0 > > + gcc_assert (first_arg); > > + #endif > > > Let's leave this out. Alright. > > + val =3D handle_arg(TREE_VALUE (parm), > > > Missing space before (. > > > - if (null_node_p (arg) > > - && DECL_TEMPLATE_INFO (fn) > > - && cand->template_decl > > - && !cand->explicit_targs) > > - conversion_warning =3D false; > > - > > + auto determine_conversion_warning =3D & > > + { > > + return !(null_node_p (current_arg) > > + && DECL_TEMPLATE_INFO (fn) > > + && cand->template_decl > > + && !cand->explicit_targs); > > + }; > > > I don't think this needs to be a lambda. Yeah probably a little unnecessary, I'll switch it back. > > + declaration, then the type of it's object parameter is still > > ... > > > + but due to [basic.scope.scope.3] we need to ignore it's > > ... > > > + /* Since a valid xobj parm has it's purpose cleared in find_xobj_parm > > > "its" TIL while we use an apostrophe for possessive nouns, we don't use one for the posessive its. English! > > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn) > > - && !(complain & tf_ptrmem_ok) && !flag_ms_extensions) > > + if (DECL_OBJECT_MEMBER_FUNC_P (fn) > > + && !(complain & tf_ptrmem_ok)) > > > The following code (not quoted) could share more code between the cases > if we check > > !(flag_ms_extensions && DECL_IOBJ_MEMBER_FUNC_P (fn)) > > here? Yeah I didn't like it very much when I wrote it, but I was too tired to reliably merge things together so I just wanted it to work. I will screw around with it more today. > > + else if (declarator->declarator->kind =3D=3D cdk_pointer) > > + error_at (DECL_SOURCE_LOCATION (xobj_parm), > > + /* "a pointer to function type cannot "? */ > > + "a function pointer type cannot " > > + "have an explicit object parameter"); > > > "pointer to function type", yes. > > > + /* The locations being used here are probably not correct. */ > > > Why not? I threw them in just so I could call inform, but it doesn't feel like the messages should be pointing at the parameter, but rather at the full type declaration. When I put those locations in I wasn't sure how to get the full declaration location, and I'm still not 100% confident in how to do it, so I just threw them in and moved on. > Let's clear xobj_parm after giving an error in the TYPENAME case I don't like the spirit of this very much, whats your reasoning for this? We're nearly at the end of the scope where it is last used, I think it would be more unclear if we suddenly set it to NULL_TREE near the end. It raises the question of whether that assignment actually does anything, or if we are just trying to indicate that it isn't being used anymore, but I already made sure to declare it in the deepest scope possible. That much should be sufficient for indicating it's usage, no? > > if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl)) > > || DECL_STATIC_FUNCTION_P (decl)) > > > I think this can just be if (DECL_OBJECT_MEMBER_FUNC_P (decl)). Alright, and going forward I'll try to make more changes that are consistent with this one. With that said I'm not sure it can, but I'll take a close look and if you're right I'll make that change. > > if (TREE_CODE (fntype) =3D=3D METHOD_TYPE) > > ctype =3D TYPE_METHOD_BASETYPE (fntype); > > + else if (DECL_XOBJ_MEMBER_FUNC_P (decl1)) > > + ctype =3D DECL_CONTEXT (decl1); > > > All of this can be > > if (DECL_CLASS_SCOPE_P (decl1)) > ctype =3D DECL_CONTEXT (decl1); > > I think I'm going to go ahead and clean that up now. Sounds good to me, a lot of this stuff needs small cleanups and I'm just concerned about making them too much. > > + /* 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))) > > > And this can be > > if (DECL_IOBJ_MEMBER_FUNC_P (decl1)) > > > + if (INDIRECT_TYPE_P (TREE_TYPE (object))) > > + /* The proxy variable forwards to the capture field. */ > > > This comment should stay outside the if. > > > + /* This might not be the most appropriate place for this, but I'm goi= ng > > + to hold back on changing it for the time being since we are short on > > + time. It's not really a parser error, it's more a semantic error. > > + But this is where the other errors were so... */ > > > It's OK to give semantic errors in the parser if they are always > detectable at parse time (before template instantiation). Okay, it's good to hear a definite answer to that, I'll keep it in mind. > Though maybe we could contain the xobj vs mutable and xobj vs static > diagnostics in this block: > > > + if (xobj_param) > > + { > > + quals =3D TYPE_UNQUALIFIED; > > + if (TYPE_REF_P (xobj_param) > > + && !(cp_type_quals (TREE_TYPE (xobj_param)) & TYPE_QUAL_CONST)) > > + LAMBDA_EXPR_MUTABLE_P (lambda_expr) =3D 1; > > + } > > > rather than separately in the mutable/static blocks? But the way you > have it is fine, too. Yeah I don't like the code very much, I am happy to refactor it, it's just another case of tired coding so I didn't move things around too much. > > + inform (DECL_SOURCE_LOCATION(xobj_param), > > > missing space before (xobj_param), repeated several times in the rest of > the function. Yeah tired coding. :) I'll be sure to go over everything with a fine tooth comb again before the final version of the patch. > > + /* Error reporting here is a little awkward, if the type of the > > + object parameter is deduced, we should tell them the lambda > > + is effectively already const, or to make the param const if it is > > + not, but if it is deduced and taken by value shouldn't we say > > + that it's taken by copy and won't mutate? > > + Seems right to me, but it's a little strange. */ > > > I think just omit the inform if dependent_type_p. Maybe I don't understand what a dependent type is as well as I thought, but doesn't this defeat every useful case? The most common being an xobj parameter of lambda type, which will always be deduced. Unless a template parameter does not count as a dependent type, which is not something I've ever thought about before. It does need to be better though, so I'll work on it and I'll keep what you said in mind. > > - if (!LAMBDA_EXPR_STATIC_P (lambda_expr)) > > + if (!LAMBDA_EXPR_STATIC_P (lambda_expr) > > + && !DECL_XOBJ_MEMBER_FUNC_P (fco)) > > > if (DECL_IOBJ_MEMBER_FUNC_P (fco)) > > > + /* Since a lambda's type is anonymous, we can assume an explicit obje= ct > > + parameter is unrelated... at least I believe so. I can think of a > > + paradoxical case that almost works, where a class is forward declared > > + and then defined deriving from a lambda that has an explicit object > > + parameter of that class type, but the only way that can work is with > > + decltype... okay yeah I think it can work for a struct defined in > > + block scope, but I'm leaving this as is until I can confirm my > > + hypothesis. */ > > > Like this? > > template struct A: T { }; > > template T f() > > { > int x =3D 42; > auto f1 =3D [x](this A&& self) { return x; }; > > return A{f1}(); > > } > int main() { return f(); } Oh boy, I had a feeling I was wrong. I was really hoping I was right but here it is, that definitely counts. I had tried something like this but I decided that it wasn't possible, I completely forgot about template argument deduction and passing it in like this. > > When I disable the error this crashes in register_parameter_specializatio= ns > > > + if (!TEMPLATE_PARM_P (non_reference (TREE_TYPE (xobj_param)))) > > > ...so let's just check dependent_type_p here. I think this jogs my memory on what a dependent type is. I believe I see how this would work now. Mildly related, a lot of the stuff I hacked together with multiple levels of accessing macros and predicates was due to not being able to find a solution for what I needed. I think we would highly benefit from better documentation of the accessors and predicates. I believe I've seen some that appear to be duplicates, and some where they don't appear to be implemented properly or match their description. If there is such a document please direct me to it as I have spent an hour or so each time I stumble on one of these problems. In the section I wrote in add_method I spent I think about 3 hours trying to figure out what combination of predicates and accessing macros was correct for what I was trying to do. It gets pretty convoluted to traverse, especially when the implementations of them are rather involved. Finding non_reference was a big help for example, and I stumbled across it by pure luck. If we don't have this kind of documentation anywhere yet, I'm happy to work on it next. I've spent a lot of time looking at it all so I feel like I have a DECENT grasp on some of it. I think it would greatly enhance the status quo, and might even help us figure out what needs to be cleaned up, removed, replaced, etc. Side side note, if said document does exist already, I either need to learn how to search the wiki more effectively or it needs some sort of improvement. > > + error ("% is unavailable for explicit object member " > > + "functions"); > > + /* 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 the > > + 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. */ > > > I still think (&self) will always be a correct replacement for 'this', > but unlikely to be the way they actually want their code to look. To > detect this-> or *this should be doable in the parser (where you can > look forward and back in the token stream) but we don't have access to > that here in finish_this_expr. Yeah, that is the issue. At the moment I'm putting any fixit stuff that I want to do on the backburner. I've come across a few more I wanted to do but, I've just come to the conclusion that learning how to do fixits properly will be something I'll need to sit down and study on its own. They are really finicky, the last one I tried to do just doesn't seem to show up at all and I don't really know why. Regarding (&self), yeah it will always be valid, it's just like you said it's unlikely to be what the user means. I don't want to put a half solution in here, I'll just delay the full solution. > > + /* Pull out the function_decl for a single xobj member function, > > + similar to the BASELINK case above. If we did the same optimization > > + as we do for single static member functions (passing in a BASELINK) > > + then it could be handled the same too. */ > > > This comment seems out of date given that the BASELINK handling just > gives an error. We can't use the same handling as static member > functions because we need the SCOPE_REF to tell us that it was properly > qualified. > > Jason I can revise how the comment reads, especially since as you've pointed out introducing the baselink optimization to non-overloaded xobj member functions would cause more problems than benefits. What I meant by the comment was that it just simply pulls the function_decl out and puts it in arg, then lets everything else after the switch case handle it. It definitely isn't clear the way it's written right now, I will revise it. I mostly got tests written yesterday, not a whole lot else done. I found that one of my comments was false specifically regarding the change in build_capture_proxy. + /* Making this conditional prevents the ICE, but does not actually work + correctly. */ So funny enough, the comment seems to be false for the version of the patch I submitted. I removed the change I made in finish_non_static_data_member before submitting the patch, but didn't realize that the change I made in there was what actually caused the problem I was referencing. It wasn't until yesterday morning that I realized that the behavior was actually correct. I also blame GDB for acting funky, for some reason even in a build with optimizations off, assignments to variables were appearing late. I blame GDB but it really might not be its fault. Regardless, it threw me off quite a bit a few times I think, I only noticed it when I started comparing return values from functions (with the finish command) and what they were being assigned to. It seems to happen when assigning to a variable that was declared but not initialized. Rambling aside, the point is, that simple change you suggested in build_capture_proxy does appear to have been sufficient for fixing the by-value xobj parameter case. The next patch will have tests to demonstrate these cases. Realistically I should have had the tests done for this version but, I just really couldn't manage it. With how much feedback you gave I think I made the right call getting it sent faster rather than waiting for all these things. I will attempt to get another version finished today. I have a feeling you were right about tsubst_lambda_expr and tsubst_function_decl so hopefully that will expedite things. As noted, the next version will include more tests so there will be less mystery in what works and what doesn't. Alex