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 D89843858D28 for ; Fri, 3 Nov 2023 04:45:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D89843858D28 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 D89843858D28 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=1698986715; cv=none; b=q34v9LcNaiWuSmG8ym0BggVsylcCWWbR2LhB7IXmNnRPHzy4wZUS5vEKRy98MM6f5uVYbq968IP6vYOteqdrTT6p6c2B6Ubl/vXBJ8OeRLo8U8mTKsaxhXHwShRvCSjEqIBionPRD1pF8kU5qdKu/X/hQ/quy3T49dTNXbJlFrk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698986715; c=relaxed/simple; bh=pXI0zkFf/rgsRCfgUJAtfCjlEb4y+Iew+8qykzBFuKk=; h=DKIM-Signature:Date:To:From:Subject:Message-ID:MIME-Version; b=gCrXO+a0zsx428jfBgZymUMAwkSECW7Dv1aCajyNF9ygLm3unMxMpzYPAqflkEb8xxLk4vK1+Gxjy/khLCljkfj0ZvnDmrpqN/breDnOOQ+WYx+nkMwda+aD68rukADl3QKuz4YiJdDGeDNiA26x78Oq1NH3qjQUtkJHgWFc40E= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1698986702; x=1699245902; bh=J46fExr3HESMQrwv5fbxQXAKuxbvCnSWodLu9udADMQ=; 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=CYFWTpiZzuMFEIjS5GTyQ3/dYwtGtQLrykelwcKj2labCmGQFg3FFS2brmGiq1uy1 aUSWYr6Gi5lL1q8OOJ2TnS6ODwOoA0zHZqRlt5ytCLPQgUoc2wI56ftiShz3BUvWlo qcziZdaWZaZXI5v14xLJTT/Ios/tQDC3oqlPYPxL5kddR2sfHkt8uervLkX4KVO/Ic D4sWCr8YLhqTbFL6mG4fqPw9CeGf1hCedyNDRMfy1SxeILEiAruvChuSRkQJz/adoP P9LZ/leXDNuNaMmoBoayYzUycvNNiOyi0JRxlgiOvjvIweHJwOeH8T4wReTSBQOLgK D7/kXodUtvCNw== Date: Fri, 03 Nov 2023 04:44:51 +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: <2f4020bf-3ef7-4627-9d92-c74676981f4b@redhat.com> 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.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: > > That leaves 2, 4, and 5. > >=20 > > 2. I am pretty sure xobj functions should have the struct they are a > > part of recorded as the method basetype member. I have already checked > > that function_type and method_type are the same node type under the > > hood and it does appear to be, so it should be trivial to set it. > > However I do have to wonder why static member functions don't set it, > > it seems to be that it would be convenient to use the same field. Can > > you provide any insight into that? >=20 >=20 > method basetype is only for METHOD_TYPE; if you want the containing type > of the function, that's DECL_CONTEXT. -- gcc/tree.h:530 #define FUNC_OR_METHOD_CHECK(T)=09TREE_CHECK2 (T, FUNCTION_TYPE, METHOD_TYP= E) -- gcc/tree.h:2518 #define TYPE_METHOD_BASETYPE(NODE)=09=09=09\ (FUNC_OR_METHOD_CHECK (NODE)->type_non_common.maxval) 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. 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. 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. I'm getting ahead of myself though, I'll stop here and avoid going on too much of a refactoring tangent. I do want this patch to make it into GCC14 after all. > > 4. I have no comment here, but it does concern me since I don't > > understand it at all. >=20 >=20 > In the list near the top of cp-tree.h, DECL_LANG_FLAG_6 for a > FUNCTION_DECL is documented to be DECL_THIS_STATIC, which should only be > set on the static member. Right, I'll try to remember to check this area in the future, but yeah that tracks because I did remove that flag. Removing that flag just so happened to be the start of this saga of bug fixes but alas, it had to be done. > > 5. I am pretty sure this is fine for now, but if xobj member functions > > ever were to support virtual/override capabilities, then it would be a > > problem. Is my understanding correct, or is there some other reason > > that iobj member functions have a different value here? >=20 >=20 > It is surprising that an iobj memfn would have a different DECL_ALIGN, > but it shouldn't be a problem; the vtables only rely on alignment being > at least 2. I'll put a note for myself to look into it in the future, it's an oddity at minimum and oddities interest me :^). > > There are also some differences in the arg param in > > cp_build_addr_expr_1 that concerns me, but most of them are reflected > > in the differences I have already noted. I had wanted to include these > > differences as well but I have been spending too much time staring at > > it, it's no longer productive. In short, the indirect_ref node for xobj > > member functions has reference_to_this set, while iobj member functions > > do not. >=20 >=20 > That's a result of difference 3. Okay, makes sense, I'm mildly concerned about any possible side effects this might have since we have a function_type node suddenly going through execution paths that only method_type went through before. The whole "reference_to_this" "pointer_to_this" thing is a little confusing because I'm pretty sure that doesn't refer to the actual `this` object argument or parameter since I've seen it all over the place. Hopefully it's benign. > > The baselink binfo field has the private flag set for xobj > > member functions, iobj member functions does not. >=20 >=20 > TREE_PRIVATE on a binfo is part of BINFO_ACCESS, which isn't a fixed > value, but gets updated during member search. Perhaps the differences > in consideration of conversion to a base led to it being set or cleared > differently? I wouldn't worry too much about it unless you see > differences in access control. 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. > > I've spent too much time on this write up, so I am calling it here, it > > wasn't all a waste of time because half of what I was doing here are > > things I was going to need to do anyway at least. I still feel like I > > spent too much time on it. Hopefully it's of some value for me/others > > later. >=20 >=20 > I hope my responses are helpful as well. Very much so, thank you! An update on the regression testing, I had one test fail comparing against commit a4d2b108cf234e7893322a32a7956ca24e283b05 (GCC13) and I'm not sure if I need to be concerned about it. libgomp.c++/../libgomp.c-c++-common/for-16.c execution test 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. That's all for now, thanks again. Alex