Re: [PATCH 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 8/31/23 04:33, Jakub Jelinek wrote:
On Thu, Aug 31, 2023 at 06:02:36AM +, waffl3x via Gcc-patches wrote:
+++ 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.
I don't think there's any obstacle to allowing it as an extension in
older standards (with a pedwarn, of course).
Jason
Re: [PATCH 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
Hey Jakub, thanks for the response and criticism, as soon as I am back at a computer I will address the issues you raised, I have a few questions though. I apologize in advanced for any errors in formatting this message, I'm writing it from a hotel room on a phone so errors are inevitable, but I'll try my best. >More importantly, should describe >what changed and not why I was under the impression that if someone wants to see the what, they can just check the diff. I don't understand what should be written if I'm just to say what, wouldn't I just be repeating what is already said in the code itself? >I think usually we don't >differentiate >in testcase names whether Yeah, for sure, but I felt like there is value in differentiating them and that it would be harmless to do so. If you feel like it's more important that convention is followed here I won't object, but I think this should be considered. >Isn't explicit-object-param too long >though? >explicit-this or deducing-this might >be shorter... I agree, but I felt like I should stick to the wording of the standard for it, but I don't feel strongly about that justification, so I wouldn't object to changing it. Truthfully I flip flopped many times around the names, I'll defer to whatever is decided by the maintainers on that without complaint. >> +} >> \ No newline at end of file >Please avoid these Yeah, I noticed it last minute and wasn't sure how big a deal it was. I will make sure to fix it along with everything else you noted. >Usually runtime tests don't try to >print something Yeah, I didn't like printing, but I'm not sure I like aborting either. I value granularity in testing, if one part of the test passes but another fails, you would want to know that. If you abort before the second you lose that granularity. Once again, I'll defer to the maintainers for this, but I think my points are valid here. >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. I'm glad you think so, I fully agree. I had planned to raise that once the initial patch made it in. I don't believe there is anything in the paper in particular that breaks previous standards. I can imagine there being problems if older projects tried to convert everything to use it as there are alignment differences (at least in the current patch version) between explicit object member functions and implicit object member functions. This is mostly a side effect of treating them as static functions most of the time, but I wasn't sure what would be ideal, nor how to change it. I decided that the difference was likely mainly due to virtual functions (if you know more about this, please be sure to correct me), and as virtual is not (currently) allowed, I just left it as it is. In short, I agree, and furthermore I think the syntax should be allowed with virtual just so style can be maintained. That would have greater implications than what you mentioned though and would need some extra hacks to make work, instead of just allowing what should -just- work. Thanks again for the input, I will get on it asap. Unfortunately that will be in a while, but I am determined to get this feature into GCC14 so one way or another I will make it happen. -Alex
Re: [PATCH 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Thu, Aug 31, 2023 at 06:02:36AM +, 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++.d
[PATCH 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
Bootstrapped and tested on x86_64-linux with no regressions. I would like to quickly thank everyone who helped me for their patience as I learned the ropes of the codebase and toolchain. It is much appretiated and this would have been much much more difficult without the support. This patch handles the new explicit object member functions by bypassing special behavior of implicit object member functions, but opting back into the special behavior during a function call through member access. This is mainly accomplished by bypassing becoming a METHOD_TYPE and remaining as a FUNCTION_TYPE. Normally, this would be treated as a static member function, but a new explicit object member function flag is added to lang_decl_base and is set for the declaration of explicit object member functions. This sets it apart from static member functions when it is relevant, and is the criteria used to opt-in to passing the implicit object argument during a member function call. The benefit of this design is less code needs to be modified to support the new feature, as most of the semantics of explicit object member functions matches those of static member functions. There is very little left to add, and hopefully there are few bugs in the implementation despite the minimal changes. It is possible there are hidden problems with passing the implicit object argument, but none of the tests I made exhibit such a thing EXCEPT for in the pathological case as I describe below. Upon reflection, a by value explicit object parameter might be broken as well, I can't recall if there's a good test for that case. Lambdas do not work yet, but you can work around it by marking it as mutable so I suspect it could be supported with minimal changes, I just ran out of time. The other thing that does not work is the pathological case with an explicit object parameter of an unrelated type and relying on a user-defined conversion operator to cast to said type in a call to that function. You can observe the failing test for that case in explicit-object-param-valid4.C, the result is somewhat interesting, but is also why I mention that there might be hidden problems here. I selectively excluded all the diagnostics from this patch, it's possible I made a mistake and the patch will be non-functional without the addition of the diagnostics patch. If that ends up being the case, please apply the following patch that includes the diagnostics and tests and judge the functionality from that. I believe that even if I mess up this patch, there should still be value in splitting up the changes into the two patches as it should make the changes to the behavior of the compiler much more clear. With that said, I believe I didn't make any mistakes while seperating the two patches, hopefully that is the case. I left in a FIXME (in call.cc) as I noticed last minute that I made a mistake, it should be benign and removing it appears to not break anything, but I don't have time to do another bootstrap at the moment. My priority is to get eyes on the changes I've made and recieve feedback. The patch including the diagnostics will follow shortly, assuming I don't run out of time and need to rush to catch my flight :). PS: Are there any circumstances where TREE_CODE is FUNCTION_DECL but the lang_specific member is null? I have a null check for that case in DECL_IS_XOBJ_MEMBER_FUNC but I question if it's necessary. 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. gcc/cp/ChangeLog: * call.cc (add_function_candidate): (Hopefully) benign mistake (add_candidates): Treat explicit object member functions as member functions when considering candidates (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 (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 (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_DEC
