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 B69AE3858D28 for ; Sat, 4 Nov 2023 06:40:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B69AE3858D28 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 B69AE3858D28 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=1699080011; cv=none; b=FRsvU2pVlOYbE8J10pOMRqN2JgBx3sQQ9dePAchdeXug8+YZ6G9N0HrPa/qZyaVtTkwZTXq7Ymx2fKzkTxt8pVsmlVwXRjw+5dvhaFphtyvZ29SBJiDgwASiOonQ6ZozBF0B0QeZ1mvjhHjpSgGyC+5a0eox6sWWL2QgCuEZV3w= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699080011; c=relaxed/simple; bh=1RMcd58HrWaBf+DCy9VvFinM8nxWHvihu5IhJ5zjB+E=; h=DKIM-Signature:Date:To:From:Subject:Message-ID:MIME-Version; b=maa8fK91tbjLKQ0raBBIl5qyaVbqmYOoS3PpnTGJSWbkoNeyMEP+n0rerPS7WGe2tYrDB8s0xkfGNNEAQmbTSUTSdCg5fSLD4NsZHJY1UGhmVGK4AAp/fTb1bWheL3ikih/kwNVIhVqKnGcrZ/0pC9qkA2vYJdMEEWrojFzHZ0Q= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1699080007; x=1699339207; bh=N6Miv/flcQjvAOFDwcmsrfORqYedKoK1+nrD1yW6yUo=; 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=p3EXYoVL+LrOs0Cvm4C2GOpmGUcZXwhdFpMFgvHTrAleRGzBZkvgNDSGb/5mVLjeK 9Eipjfx3oseojvN9QUL90mYsF5xstIPtDfnXQqNOKB0fTTsbuM57s1b+ImYp/KR1Y9 ExlYlISGiiYoSr+A1gRIOWPtq6aavA5xB52RGa1IymJ+gy0BTgp5teyzYEnVAhR1O2 Ko6xNk2FVDJruT9bmsmv0jdr5BCfV/Fc8/aohyqjcWV2ReaPJlrfdfhtwBD5SmZK35 3qi32OHVVO7z251hWUbzNIHcbD+cfFvTw/JBzb7bGLVNpOxk309YD6AQeqXMO9xAb3 tuEre02V0pZHQ== Date: Sat, 04 Nov 2023 06:40:03 +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: <2nV4Jls2nmL2lahW6X-HZsBb90H1QZJhy3ApVC8xpxvP6KUYDriDYJpxzqjDnshOUEQ6ehYzuXU6rJmPvj3l6jtqAniFlH4Buyfls4E8cfY=@protonmail.com> In-Reply-To: References: <1fdadb6b-e4ca-40c1-bb1c-43a0f42826ba@redhat.com> <635f5d6d-2807-4dff-8a37-73d323d6ea97@redhat.com> <2f4020bf-3ef7-4627-9d92-c74676981f4b@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=-2.7 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,WEIRD_PORT 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: I'm unfortunately going down a rabbit hole again. --function.h:608 ``` /* If pointers to member functions use the least significant bit to 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)=09 \ ? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY) ``` --decl.cc:10400 grokfndecl ``` if (TREE_CODE (type) =3D=3D METHOD_TYPE) { tree parm =3D build_this_parm (decl, type, quals); DECL_CHAIN (parm) =3D parms; parms =3D parm; /* Allocate space to hold the vptr bit if needed. */ SET_DECL_ALIGN (decl, MINIMUM_METHOD_BOUNDARY); } ``` The good news is I think I found where the alignment is being set, so it can be addressed later if desired. I stumbled upon this while cleaning up the patch, grokfndecl is just so full of cruft it's crazy hard to reason about. There's more than one block that I am near certain is completely dead code. I would like to just ignore them for now but some of them unfortunately pertain to xobj functions. I just don't feel good about putting in any hacks, but to really get any modifications in here correct it would need to be refactored much more than I should be doing in this patch. Here's another example that I'm not sure how I want to address it. ~decl.cc:10331 grokfndecl ``` int staticp =3D ctype && TREE_CODE (type) =3D=3D FUNCTION_TYPE; ``` ~decl.cc:10506 grokfndecl ``` /* If this decl has namespace scope, set that up. */ if (in_namespace) set_decl_namespace (decl, in_namespace, friendp); else if (ctype) DECL_CONTEXT (decl) =3D ctype; else DECL_CONTEXT (decl) =3D FROB_CONTEXT (current_decl_namespace ()); ``` And just a few lines down; ~decl.cc:10529 ``` /* Should probably propagate const out from type to decl I bet (mrs). */ if (staticp) { DECL_STATIC_FUNCTION_P (decl) =3D 1; DECL_CONTEXT (decl) =3D ctype; } ``` If staticp is true, ctype must have been non-null, and if ctype is non-null, the context for decl should have been set in the second block. So why was the code in the second block added? commit f3665bd1111c1799c0421490b5e655f977570354 Author: Nathan Sidwell Date: Tue Jul 28 08:57:36 2020 -0700 c++: Set more DECL_CONTEXTs =20 I discovered we were not setting DECL_CONTEXT in a few cases, and grokfndecl's control flow wasn't making it clear that we were doing it in all cases. =20 gcc/cp/ * cp-gimplify.c (cp_genericize_r): Set IMPORTED_DECL's context. * cp-objcp-common.c (cp_pushdecl): Set decl's context. * decl.c (grokfndecl): Make DECL_CONTEXT setting clearer. According to the commit, it was because it was not clear, which quite frankly I can agree to, it just wasn't determined that the code below is redundantly setting the context so it wasn't removed. This puts me in a dilemma though, do I put another condition in that code block for the xobj case even though the code is nearly dead? Or do I give it a minor refactor for it to make a little more sense? If I add to the code I feel like it's just going to add to the problem, while if I give it a minor refactor it still won't look great and has a greater chance of breaking something. In this case I'm going to risk refactoring it, staticp is only used in that 1 place so I will just rip it out. I am not concerned with decl's type spontaneously changing to something that is not FUNCTION_TYPE, and if it did I think there are bigger problems afoot. I guess I'll know if I went too far with the refactoring when the patch reaches you, do let me know about this one specifically though because it took up a lot of my time trying to decide how to address it. > > The code doesn't seem to reflect that, perhaps since the underlying > > node type is the same (as far as I can tell, they both inherit from > > tree_type_non_common) it wasn't believed to be necessary. >=20 >=20 > Curious. It might also be a remnant of how older code dealt with how > sometimes a member function changes between FUNCTION_TYPE and > METHOD_TYPE during parsing. Sounds plausible. > > Upon looking at DECL_CONTEXT though I see it seems you were thinking of > > FUNCTION_DECL. I haven't observed DECL_CONTEXT being set for > > FUNCTION_DECL nodes though, perhaps it should be? Although it's more > > likely that it is being set and I just haven't noticed, but if that's > > the case I'll have to make a note to make sure it is being set for xobj > > member functions. >=20 >=20 > I would certainly expect it to be getting set already. This being on my mind is partially what sent me down the rabbit hole above, but yeah, it seems like it is. > > I was going to say that this seems like a redundancy, but I realized > > that the type of a member function pointer is tied to the struct, so it > > actually ends up relevant for METHOD_TYPE nodes. I would hazard a guess > > that when forming member function pointers the FUNCTION_DECL node was > > not as easily accessed. If I remember correctly that is not the case > > right now so it might be worthwhile to refactor away from > > TYPE_METHOD_BASETYPE and replace uses of it with DECL_CONTEXT. >=20 >=20 > For a pointer-to-member, the class type is part of the type, so trying > to remove it from the type doesn't sound like an improvement to me. > Specifically, TYPE_PTRMEM_CLASS_TYPE refers to TYPE_METHOD_BASETYPE for > a pointer to member function. I suppose it's not a good idea given that other decl nodes use the same field, but redundancies do feel icky to me. It makes stuff way harder to reason about for me. > >=20 > > Unfortunately I don't have any tests for private/public access yet, > > it's one of the area's I've neglected. Unfortunately I probably won't > > put too much effort into writing TOO many more right now as it takes up > > a lot of my time. I still have to clean up the ones I currently have > > and I have a few I wanted to write that are not yet written. >=20 >=20 > Makes sense. I wouldn't expect access control to need specific changes. I think I saw something that concerned me regarding this in grokfndecl, but I agree that it shouldn't need specific changes, so I'm going to assume I imagined it unless problems start to pop up :). > No, that test has been pretty flaky for me, you can ignore it. Noted. > > I'm going to be starting tests for my patch against trunk now, once > > that is finished I should be ready to format a patch for review. >=20 >=20 > Great! All tests seemed to pass when applied to GCC14, but the results did something funny where it said tests disappeared and new tests appeared and passed. The ones that disappeared and the new ones that appeared 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. Back to work now, thank you as always. Alex