From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 449CC3858D37 for ; Thu, 31 Aug 2023 08:33:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 449CC3858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1693470817; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=cQmWsYBI/S7ReNY4S0hegqOjVPm44tonspbhCUn8nGg=; b=CR+aqh591dAB3z8agsTF/2+PZVi5RagJlf7WnpdqhID44cZNQxmf/StsbO7MQWBvyLKkrQ 4YvgwPBvyPy59vJHHWft1aJ1gshSWJkzF9eUVEHP0C1TzaManI/+rkjys4M9gRxHfbUBv/ NVZYwi7fbxIyxuwPya5To2ie0PG5V8M= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-577-vJd4lbRjOZukccJC3U8fQw-1; Thu, 31 Aug 2023 04:33:36 -0400 X-MC-Unique: vJd4lbRjOZukccJC3U8fQw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1F725803E2E; Thu, 31 Aug 2023 08:33:36 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.45.224.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D88DB40C2063; Thu, 31 Aug 2023 08:33:35 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 37V8XYuI265222 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 31 Aug 2023 10:33:34 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 37V8XXVD265221; Thu, 31 Aug 2023 10:33:33 +0200 Date: Thu, 31 Aug 2023 10:33:33 +0200 From: Jakub Jelinek To: waffl3x , Jason Merrill Cc: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609] Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP 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 Thu, Aug 31, 2023 at 06:02:36AM +0000, waffl3x via Gcc-patches wrote: > From e485a79ec5656e72ba46053618843c3d69331eab Mon Sep 17 00:00:00 2001 > From: Waffl3x > Date: Thu, 31 Aug 2023 01:05:25 -0400 > Subject: [PATCH] P0847R7 (deducing this) Initial support > > Most things should be functional, lambdas need a little more work though. > Limitations: Missing support for lambdas, and user defined conversion functions when passing the implicit object argument does not work properly. See explicit-object-param-valid4.C for an example of the current (errent) behavior. > > There is a slight mistake in call.cc, it should be benign. Thanks for working on this. Just some random mostly formatting comments, defering the actual patch review to Jason. The ChangeLog should refer to PR c++/102609 and ideally mention somewhere that it implements C++23 P0847R7 - Deducing this. so that when one quickly searches when that was implemented it can be found easily. ChangeLog entries should start with capital letter after ): and end with . And they should fit into 80 columns, one can wrap lines (but use tab indentation even on the subsequent lines). More importantly, should describe what changed and not why, if the why needs explanation, it should go into comments in the code. > gcc/cp/ChangeLog: > > * call.cc (add_function_candidate): (Hopefully) benign mistake So, this both misses . at the end and doesn't describe what changed. * call.cc (add_function_candidate): Don't call build_this_conversion for DECL_IS_XOBJ_MEMBER_FUNC. ? > (add_candidates): Treat explicit object member functions as member functions when considering candidates Too long line and missing . at the end > (build_over_call): Enable passing an implicit object argument when calling an explicit object member function > * cp-tree.h (struct lang_decl_base): Added member xobj_flag for differentiating explicit object member functions from static member functions Just mention that xobj_flag member has been added, not what it is for. > (DECL_FUNC_XOBJ_FLAG): New, checked access for xobj_flag > (DECL_PARM_XOBJ_FLAG): New, access decl_flag_3 > (DECL_IS_XOBJ_MEMBER_FUNC): New, safely check if a node is an explicit object member function These are macros, just say Define. etc. > (enum cp_decl_spec): Support parsing 'this' as a decl spec, change is mirrored in parser.cc:set_and_check_decl_spec_loc > * decl.cc (grokfndecl): Sets the xobj flag for the FUNCTION_DECL if the first parameter is an explicit object parameter > (grokdeclarator): Sets the xobj flag for PARM_DECL if 'this' spec is present in declspecs, bypasses conversion from FUNCTION_DECL to METHOD_DECL if an xobj flag is set for the first parameter of the given function declarator > * parser.cc (cp_parser_decl_specifier_seq): check for 'this' specifier > (set_and_check_decl_spec_loc): extended decl_spec_names to support 'this', change is mirrored in cp-tree.h:cp_decl_spec > > gcc/ChangeLog: > > * tree-core.h (struct tree_decl_common): Added comment describing new use of decl_flag_3 > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp23/explicit-object-param-valid1.C: New test. > * g++.dg/cpp23/explicit-object-param-valid2.C: New test. > * g++.dg/cpp23/explicit-object-param-valid3.C: New test. > * g++.dg/cpp23/explicit-object-param-valid4.C: New test. I think usually we don't differentiate in testcase names whether the test is to be accepted or rejected. So, one would just go with explicit-object-param{1,2,3,4,5,6}.C etc. for everything related to the feature. Isn't explicit-object-param too long though? explicit-this or deducing-this might be shorter... > + /* FIXME: This doesn't seem to be neccesary, upon review I > + realized that it doesn't make sense (an xobj member func > + is not a nonstatic_member_function, so this check will > + never change anything) */ Comments should end with a dot and two spaces before */ > @@ -9995,7 +10001,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)) == METHOD_TYPE) > + else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE > + || DECL_IS_XOBJ_MEMBER_FUNC (fn) ) No space between fn) and ) > +/* these need to moved to somewhere appropriate */ Comments should start with a capital letter and like above, end with appropriate. */ > + && DECL_LANG_SPECIFIC (STRIP_TEMPLATE (NODE))->u.base.xobj_flag == 1) \ No \ after the last line of the macro. > +} > \ No newline at end of file Please avoid these (unless testing preprocessor etc. that it can handle even sources which don't end with a newline). > diff --git a/gcc/testsuite/g++.dg/cpp23/explicit-object-param-valid2.C b/gcc/testsuite/g++.dg/cpp23/explicit-object-param-valid2.C > new file mode 100644 > index 00000000000..2f9a08207d4 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp23/explicit-object-param-valid2.C > @@ -0,0 +1,24 @@ > +// P0847R7 > +// { dg-do run { target c++23 } } This raises an important question whether we as an extension should support deducing this even in older standards or not. I admit I haven't studied the paper enough to figure that out. The syntax is certainly something that wasn't valid in older standards, so from that POV it could be accepted say with pedwarn with OPT_Wc__23_extensions if cxx_dialect < cxx23. But perhaps some of the rules in the paper change something unconditionally even when the new syntax doesn't appear. And, if it is accepted in older standards, the question is if it shouldn't be banned say from C++98. > +// explicit object member function pointer type deduction and conversion to function pointer > +// and calling through pointer to function > + > +struct S { > + int _n; > + int f(this S& self) { return self._n; } > +}; > + > +using f_type = int(*)(S&); > + > +static_assert(__is_same(f_type, decltype(&S::f))); > + > +int main() > +{ > + auto fp0 = &S::f; > + f_type fp1 = &S::f; > + static_assert(__is_same(decltype(fp0), decltype(fp1))); > + S s{42}; > + // { dg-output "42" } > + __builtin_printf("%d\n%d\n", fp0(s), fp1(s)); Usually runtime tests don't try to print something with dg-output trying to match it, but instead just compare the values directly and __builtin_abort () if something has incorrect value. Also, in the above case, dg-output matches 42 anywhere in the output, so if fp0(s) is 42 or if fp1(s) is 42. I'd expect if (fp0 (s) != 42 || fp1 (s) != 42) __builtin_abort (); Jakub