Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Monday, November 20th, 2023 at 7:35 AM, Jason Merrill
wrote:
>
>
> On 11/19/23 16:44, waffl3x wrote:
>
> > On Sunday, November 19th, 2023 at 1:34 PM, Jason Merrill ja...@redhat.com
> > wrote:
> >
> > > On 11/19/23 13:36, waffl3x wrote:
> > >
> > > > I'm having trouble fixing the error for this case, the control flow
> > > > when the functions are overloaded is much more complex.
> > > >
> > > > struct S {
> > > > void f(this S&) {}
> > > > void f(this S&, int)
> > > >
> > > > void g() {
> > > > void (*fp)(S&) =
> > > > }
> > > > };
> > > >
> > > > This seemed to have fixed the non overloaded case, but I'm also not
> > > > very happy with it, it feels kind of icky. Especially since the expr's
> > > > location isn't available here, although, it just occurred to me that
> > > > the expr's location is probably stored in the node.
> > > >
> > > > typeck.cc:cp_build_addr_expr_1
> > > > ```
> > > > case BASELINK:
> > > > arg = BASELINK_FUNCTIONS (arg);
> > > > if (DECL_XOBJ_MEMBER_FUNC_P (
> > > > {
> > > > error ("You must qualify taking address of xobj member functions");
> > > > return error_mark_node;
> > > > }
> > >
> > > The loc variable was set earlier in the function, you can use that.
> >
> > Will do.
> >
> > > The overloaded case we want to handle here in
> > > resolve_address_of_overloaded_function:
> > >
> > > > if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
> > > > && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
> > > > {
> > > > static int explained;
> > > >
> > > > if (!(complain & tf_error))
> > > > return error_mark_node;
> > > >
> > > > auto_diagnostic_group d;
> > > > if (permerror (input_location, "assuming pointer to member %qD", fn)
> > > > && !explained)
> > > > {
> > > > inform (input_location, "(a pointer to member can only be "
> > > > "formed with %<&%E%>)", fn);
> > > > explained = 1;
> > > > }
> > > > }
> > >
> > > Jason
> >
> > I'll check that out now, I just mostly finished the first lambda crash.
> >
> > What is the proper way to error out of instantiate_body? What I have
> > right now is just not recursing down further if theres a problem. Also,
> > I'm starting to wonder if I should actually be erroring in
> > instantiate_decl instead.
>
>
> I think you want to error in start_preparsed_function, to handle
> template and non-template cases in the same place.
>
> Jason
I just started a bootstrap, hopefully everything comes out just fine.
If I don't pass out before the tests finish (and the tests are all
fine) then I'll send it in for review tonight.
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.
This patch is not as final as I would have liked as you can probably
deduce from the previous paragraph. I still have to write tests for a
number of cases, but I'm pretty sure everything works. I was going to
say except for by-value xobj parameters in lambdas with captures, but
that's magically working now too. I was also going to say I don't know
why, but I found where my mistake was (that I fixed without realizing
it was causing the aforementioned problem) so I do know what did it.
So rambling aside, I think everything should work now. To reiterate, I
still have to finish the tests for a few things. There's some
diagnostics I'm super not happy with, and lambda's names still say
static, but I already know how to fix that I think.
I will make an attempt at moving the diagnostics for an unrelated
explicit object parameter in a lambda with captures tomorrow. I just
want to get the almost fully featured patch reviewed ASAP, even if it's
still got some cruft.
As soon as these tests pass I will submit the patch, I'm not going to
split it up today, I'm simply too tired, but I assure you the final
version will properly be split up with a correct commit message and
everything.
Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 11/19/23 16:44, waffl3x wrote:
On Sunday, November 19th, 2023 at 1:34 PM, Jason Merrill
wrote:
On 11/19/23 13:36, waffl3x wrote:
I'm having trouble fixing the error for this case, the control flow
when the functions are overloaded is much more complex.
struct S {
void f(this S&) {}
void f(this S&, int)
void g() {
void (*fp)(S&) =
}
};
This seemed to have fixed the non overloaded case, but I'm also not
very happy with it, it feels kind of icky. Especially since the expr's
location isn't available here, although, it just occurred to me that
the expr's location is probably stored in the node.
typeck.cc:cp_build_addr_expr_1
```
case BASELINK:
arg = BASELINK_FUNCTIONS (arg);
if (DECL_XOBJ_MEMBER_FUNC_P (
{
error ("You must qualify taking address of xobj member functions");
return error_mark_node;
}
The loc variable was set earlier in the function, you can use that.
Will do.
The overloaded case we want to handle here in
resolve_address_of_overloaded_function:
if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
&& !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
{
static int explained;
if (!(complain & tf_error))
return error_mark_node;
auto_diagnostic_group d;
if (permerror (input_location, "assuming pointer to member %qD", fn)
&& !explained)
{
inform (input_location, "(a pointer to member can only be "
"formed with %<&%E%>)", fn);
explained = 1;
}
}
Jason
I'll check that out now, I just mostly finished the first lambda crash.
What is the proper way to error out of instantiate_body? What I have
right now is just not recursing down further if theres a problem. Also,
I'm starting to wonder if I should actually be erroring in
instantiate_decl instead.
I think you want to error in start_preparsed_function, to handle
template and non-template cases in the same place.
Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Sunday, November 19th, 2023 at 1:34 PM, Jason Merrill
wrote:
>
>
> On 11/19/23 13:36, waffl3x wrote:
>
> > I'm having trouble fixing the error for this case, the control flow
> > when the functions are overloaded is much more complex.
> >
> > struct S {
> > void f(this S&) {}
> > void f(this S&, int)
> >
> > void g() {
> > void (*fp)(S&) =
> > }
> > };
> >
> > This seemed to have fixed the non overloaded case, but I'm also not
> > very happy with it, it feels kind of icky. Especially since the expr's
> > location isn't available here, although, it just occurred to me that
> > the expr's location is probably stored in the node.
> >
> > typeck.cc:cp_build_addr_expr_1
> > ```
> > case BASELINK:
> > arg = BASELINK_FUNCTIONS (arg);
> > if (DECL_XOBJ_MEMBER_FUNC_P (
> > {
> > error ("You must qualify taking address of xobj member functions");
> > return error_mark_node;
> > }
>
>
> The loc variable was set earlier in the function, you can use that.
Will do.
> The overloaded case we want to handle here in
> resolve_address_of_overloaded_function:
>
> > if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
> > && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
> > {
> > static int explained;
> >
> > if (!(complain & tf_error))
> > return error_mark_node;
> >
> > auto_diagnostic_group d;
> > if (permerror (input_location, "assuming pointer to member %qD", fn)
> > && !explained)
> > {
> > inform (input_location, "(a pointer to member can only be "
> > "formed with %<&%E%>)", fn);
> > explained = 1;
> > }
> > }
>
>
> Jason
I'll check that out now, I just mostly finished the first lambda crash.
What is the proper way to error out of instantiate_body? What I have
right now is just not recursing down further if theres a problem. Also,
I'm starting to wonder if I should actually be erroring in
instantiate_decl instead.
I guess it will be better to just finish and you can share your
comments upon review though.
Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 11/19/23 13:36, waffl3x wrote:
I'm having trouble fixing the error for this case, the control flow
when the functions are overloaded is much more complex.
struct S {
void f(this S&) {}
void f(this S&, int)
void g() {
void (*fp)(S&) =
}
};
This seemed to have fixed the non overloaded case, but I'm also not
very happy with it, it feels kind of icky. Especially since the expr's
location isn't available here, although, it just occurred to me that
the expr's location is probably stored in the node.
typeck.cc:cp_build_addr_expr_1
```
case BASELINK:
arg = BASELINK_FUNCTIONS (arg);
if (DECL_XOBJ_MEMBER_FUNC_P (
{
error ("You must qualify taking address of xobj member functions");
return error_mark_node;
}
The loc variable was set earlier in the function, you can use that.
The overloaded case we want to handle here in
resolve_address_of_overloaded_function:
if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
&& !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
{
static int explained;
if (!(complain & tf_error))
return error_mark_node;
auto_diagnostic_group d;
if (permerror (input_location, "assuming pointer to member %qD", fn)
&& !explained)
{
inform (input_location, "(a pointer to member can only be "
"formed with %<&%E%>)", fn);
explained = 1;
}
}
Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Sunday, November 19th, 2023 at 9:31 AM, Jason Merrill
wrote:
>
>
> On 11/19/23 08:39, waffl3x wrote:
>
> > Funny enough I ended up removing the ones I was thinking about, seems
> > to always happen when I ask style questions but I'm glad to hear it's
> > okay going forward.
> >
> > I'm having trouble fixing this bug, based on what Gasper said in
> > PR102609 I am pretty sure I know what the semantics should be. Since
> > the capture is not used in the body of the function, it should be well
> > formed to call the function with an unrelated type.
>
>
> I don't think so: https://eel.is/c++draft/expr.prim#lambda.closure-5
> says the type of the xobj parameter must be related.
Well, thanks for bringing that to my attention, that makes things
easier but I'm kinda disappointed, I almost wanted an excuse to write
more code. I wonder why Gasper thought this wasn't the case, perhaps it
was a later decision?
Okay, I checked, that paragraph is in the original paper, I thought I
could just take what one of the paper's authors said for granted but I
guess not, I'm not going to make that mistake again. On the other hand,
maybe he just misunderstood my question, what can you do.
Well regardless, that reduces the things I have left to do by a whole
lot.
> > I had begun trying to tackle the case that Gasper mentioned and got the
> > following ICE. I also have another case that ICEs so I've been thinking
> > I don't get to do little changes to fix this. I've been looking at this
> > for a few hours now and given we are past the deadline now I figured I
> > should see what others think.
> >
> > int main()
> > {
> > int x = 42;
> > auto f1 = [x](this auto&& self) {};
> >
> > static_cast(decltype(f1)::operator());
> > }
>
>
> This should be rejected when we try to instantiate the op() with int.
Yep, absolutely, that is clear now.
Just for the record, clang accepts it, but since I can't think of any
use cases for this I don't think there's any value in supporting it.
>
> > As I said, there is also this case that also ICEs in the same region.
> > It's making me think that some core assumptions are being violated in
> > the code leading up to finish_non_static_data_member.
> >
> > int main()
> > {
> > int x = 42;
> > auto f1 = [x](this auto self) {};
> > }
>
>
> Here I think the problem is in build_capture_proxy:
>
> > /* The proxy variable forwards to the capture field. */
> > object = build_fold_indirect_ref (DECL_ARGUMENTS (fn));
> > object = finish_non_static_data_member (member, object, NULL_TREE);
>
>
> The call to build_fold_indirect_ref assumes that 'this' is a pointer,
> which it is not here. I think you can just make that conditional on it
> being a pointer or reference?
>
> Jason
Thanks, I will take a look at that area.
I'm having trouble fixing the error for this case, the control flow
when the functions are overloaded is much more complex.
struct S {
void f(this S&) {}
void f(this S&, int)
void g() {
void (*fp)(S&) =
}
};
This seemed to have fixed the non overloaded case, but I'm also not
very happy with it, it feels kind of icky. Especially since the expr's
location isn't available here, although, it just occurred to me that
the expr's location is probably stored in the node.
typeck.cc:cp_build_addr_expr_1
```
case BASELINK:
arg = BASELINK_FUNCTIONS (arg);
if (DECL_XOBJ_MEMBER_FUNC_P (
{
error ("You must qualify taking address of xobj member functions");
return error_mark_node;
}
Anyway, I'm quite tired but I'll to finish off the lambda stuff before
calling it, then I'll run a bootstrap and tests and if all is well I
will submit the patch. I will probably skimp on the changelog and
commit message as that's the part I have the hardest time on,
especially when I'm tired.
Alex
```
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 11/19/23 08:39, waffl3x wrote:
Funny enough I ended up removing the ones I was thinking about, seems
to always happen when I ask style questions but I'm glad to hear it's
okay going forward.
I'm having trouble fixing this bug, based on what Gasper said in
PR102609 I am pretty sure I know what the semantics should be. Since
the capture is not used in the body of the function, it should be well
formed to call the function with an unrelated type.
I don't think so: https://eel.is/c++draft/expr.prim#lambda.closure-5
says the type of the xobj parameter must be related.
I had begun trying to tackle the case that Gasper mentioned and got the
following ICE. I also have another case that ICEs so I've been thinking
I don't get to do little changes to fix this. I've been looking at this
for a few hours now and given we are past the deadline now I figured I
should see what others think.
int main()
{
int x = 42;
auto f1 = [x](this auto&& self) {};
static_cast(decltype(f1)::operator());
}
This should be rejected when we try to instantiate the op() with int.
As I said, there is also this case that also ICEs in the same region.
It's making me think that some core assumptions are being violated in
the code leading up to finish_non_static_data_member.
int main()
{
int x = 42;
auto f1 = [x](this auto self) {};
}
Here I think the problem is in build_capture_proxy:
/* The proxy variable forwards to the capture field. */
object = build_fold_indirect_ref (DECL_ARGUMENTS (fn));
object = finish_non_static_data_member (member, object, NULL_TREE);
The call to build_fold_indirect_ref assumes that 'this' is a pointer,
which it is not here. I think you can just make that conditional on it
being a pointer or reference?
Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
Funny enough I ended up removing the ones I was thinking about, seems
to always happen when I ask style questions but I'm glad to hear it's
okay going forward.
I'm having trouble fixing this bug, based on what Gasper said in
PR102609 I am pretty sure I know what the semantics should be. Since
the capture is not used in the body of the function, it should be well
formed to call the function with an unrelated type.
I had begun trying to tackle the case that Gasper mentioned and got the
following ICE. I also have another case that ICEs so I've been thinking
I don't get to do little changes to fix this. I've been looking at this
for a few hours now and given we are past the deadline now I figured I
should see what others think.
int main()
{
int x = 42;
auto f1 = [x](this auto&& self) {};
static_cast(decltype(f1)::operator());
}
explicit-obj-lambdaX3.C: In instantiation of 'main():: static
[with auto:1 = int&]':
explicit-obj-lambdaX3.C:33:53: required from here
33 | static_cast(decltype(f1)::operator());
| ^
explicit-obj-lambdaX3.C:31:33: internal compiler error: tree check: expected
record_type or union_type or qual_union_type, have integer_type in
finish_non_static_data_member, at cp/semantics.cc:2294
31 | auto f1 = [x](this auto&& self) {};
| ^
0x1c66dda tree_check_failed(tree_node const*, char const*, int, char const*,
...)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/tree.cc:8949
0xb2e125 tree_check3(tree_node*, char const*, int, char const*, tree_code,
tree_code, tree_code)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/tree.h:3638
0xedfaf4 finish_non_static_data_member(tree_node*, tree_node*, tree_node*, int)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/semantics.cc:2294
0xe8b9b8 tsubst_expr(tree_node*, tree_node*, int, tree_node*)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/pt.cc:20864
0xe6d713 tsubst_decl
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/pt.cc:15387
0xe6fb1b tsubst(tree_node*, tree_node*, int, tree_node*)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/pt.cc:15967
0xe7bd81 tsubst_stmt
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/pt.cc:18299
0xe7df18 tsubst_stmt
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/pt.cc:18554
0xea6982 instantiate_body
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/pt.cc:26743
0xea83e9 instantiate_decl(tree_node*, bool, bool)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/pt.cc:27030
0xb5f9c9 resolve_address_of_overloaded_function
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/class.cc:8802
0xb60be1 instantiate_type(tree_node*, tree_node*, int)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/class.cc:9061
0xaf9992 standard_conversion
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/call.cc:1244
0xafcb57 implicit_conversion
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/call.cc:2081
0xb2a8cb perform_direct_initialization_if_possible(tree_node*, tree_node*,
bool, int)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/call.cc:13456
0xf69db8 build_static_cast_1
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/typeck.cc:8356
0xf6af1b build_static_cast(unsigned int, tree_node*, tree_node*, int)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/typeck.cc:8566
0xd9fc02 cp_parser_postfix_expression
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/parser.cc:7531
0xda45af cp_parser_unary_expression
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/parser.cc:9244
0xda5db4 cp_parser_cast_expression
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/parser.cc:10148
As I said, there is also this case that also ICEs in the same region.
It's making me think that some core assumptions are being violated in
the code leading up to finish_non_static_data_member.
int main()
{
int x = 42;
auto f1 = [x](this auto self) {};
}
explicit-obj-lambdaX3.C: In lambda function:
explicit-obj-lambdaX3.C:31:31: internal compiler error: Segmentation fault
31 | auto f1 = [x](this auto self) {};
| ^
0x1869eaa crash_signal
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/toplev.cc:315
0xb2ea0b strip_array_types(tree_node*)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/tree.h:4955
0xf773e2 cp_type_quals(tree_node const*)
/home/waffl3x/projects/gcc-dev/workspace/src/xobj-next/gcc/cp/typeck.cc:11509
0xedf993 finish_non_static_data_member(tree_node*, tree_node*, tree_node*, int)
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Sat, Nov 18, 2023 at 1:43 AM waffl3x wrote:
>
> The patch is coming along, I just have a quick question regarding
> style. I make use of IILE's (immediately invoked lambda expression) a
> whole lot in my own code. I know that their use is controversial in
> general so I would prefer to ask instead of just submitting the patch
> using them a bunch suddenly. I wouldn't have bothered either but this
> part is really miserable without them.
We don't use them much currently, but I'm not opposed to them either,
if they help make the code clearer.
> If that would be okay, I would suggest an additional exception to
> bracing style for lambdas.
> This:
> [](){
> // stuff
> };
> Instead of this:
> []()
> {
> // stuff
> };
Makes sense.
Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
The patch is coming along, I just have a quick question regarding
style. I make use of IILE's (immediately invoked lambda expression) a
whole lot in my own code. I know that their use is controversial in
general so I would prefer to ask instead of just submitting the patch
using them a bunch suddenly. I wouldn't have bothered either but this
part is really miserable without them.
If that would be okay, I would suggest an additional exception to
bracing style for lambdas.
This:
[](){
// stuff
};
Instead of this:
[]()
{
// stuff
};
This is especially important for IILE pattern IMO, else it looks really
mediocre. If this isn't okay okay I'll refactor all the IILE's that I
added, or just name them and call them instead. Whatever you think is
most appropriate.
Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Monday, November 13th, 2023 at 8:48 PM, Jason Merrill
wrote:
>
>
> On 11/11/23 05:43, waffl3x wrote:
>
> > > [combined reply to all three threads]
> > >
> > > On 11/9/23 23:24, waffl3x wrote:
> > >
> > > > > > There are a few known issues still present in this patch. Most
> > > > > > importantly,
> > > > > > the implicit object argument fails to convert when passed to
> > > > > > by-value xobj
> > > > > > parameters. This occurs both for xobj parameters that match the
> > > > > > argument type
> > > > > > and xobj parameters that are unrelated to the object type, but have
> > > > > > valid
> > > > > > conversions available. This behavior can be observed in the
> > > > > > explicit-obj-by-value[1-3].C tests. The implicit object argument
> > > > > > appears to be
> > > > > > simply reinterpreted instead of any conversion applied. This is
> > > > > > elaborated on
> > > > > > in the test cases.
> > > > >
> > > > > Yes, that's because of:
> > > > >
> > > > > > @@ -9949,7 +9951,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_XOBJ_MEMBER_FUNC_P (fn))
> > > > >
> > > > > We don't want to take this path for xob fns. Instead I think we need
> > > > > to
> > > > > change the existing:
> > > > >
> > > > > > gcc_assert (first_arg == NULL_TREE);
> > > > >
> > > > > to assert that if first_arg is non-null, we're dealing with an xob fn,
> > > > > and then go ahead and do the same conversion as the loop body on
> > > > > first_arg.
> > > > >
> > > > > > Despite this, calls where there is no valid conversion
> > > > > > available are correctly rejected, which I find surprising. The
> > > > > > explicit-obj-by-value4.C testcase demonstrates this odd but correct
> > > > > > behavior.
> > > > >
> > > > > Yes, because checking for conversions is handled elsewhere.
> > > >
> > > > Yeah, as I noted above I realized that just handling it the same way as
> > > > iobj member functions is fundamentally broken. I was staring at it last
> > > > night and eventually realized that I could just copy the loop body. I
> > > > ended up asserting in the body handling the implicit object argument
> > > > for xobj member functions that first_arg != NULL_TREE, which I wasn't
> > > > sure of, but it seems to work.
> > >
> > > That sounds like it might cause trouble with
> > >
> > > struct A {
> > > void f(this A);
> > > };
> > >
> > > int main()
> > > {
> > > (::f) (A());
> > > }
> >
> > I will check to see what the behavior with this is. This sounds related
> > to the next question I asked as well.
> >
> > > > I tried asking in IRC if there are any circumstances where first_arg
> > > > would be null for a non-static member function and I didn't get an
> > > > answer. The code above seemed to indicate that it could be. It just
> > > > looks like old code that is no longer valid and never got removed.
> > > > Consequently this function has made it on my list of things to refactor
> > > > :^).
> > >
> > > Right, first_arg is only actually used for the implicit object argument,
> > > it's just easier to store it separately from the arguments in (). I'm
> > > not sure which code you mean is no longer valid?
> >
> > Yeah I agree that it's easier to store it separately.
> >
> > -- call.cc:build_over_call
> > `else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE) { tree arg =
> > build_this (first_arg != NULL_TREE ? first_arg : (*args)[arg_index]);`
> >
> > The trouble is, the code (shown above) does not assume that this holds
> > true. It handles the case where the implicit object argument was passed
> > in with the rest of the arguments. As far as I've observed, it seems
> > like it's always passed in through the first_arg member of cand, which
> > is what I was referring to here.
> >
> > > > ended up asserting in the body handling the implicit object argument
> > > > for xobj member functions that first_arg != NULL_TREE, which I wasn't
> > > > sure of, but it seems to work.
> >
> > Since it wasn't clear what I was referring to, here is the code that I
> > wrote (copied from the loop really) handling the case. In case it isn't
> > obvious, I didn't snip the code in the METHOD_TYPE block, it's just
> > snipped here as it's not code I've modified. I'm hopeful that the case
> > you mentioned above is not problematic, but like I said I will be sure
> > to test it.
> >
> > -- call.cc:build_over_call
> > ```
> > else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
> > {
> > /* SNIP */
> > if (first_arg != NULL_TREE)
> > first_arg = NULL_TREE;
> > else
> > ++arg_index;
> > ++i;
> > is_method = 1;
> > }
> > else if (DECL_XOBJ_MEMBER_FUNC_P (fn))
> > {
> > gcc_assert (cand->first_arg);
> > gcc_assert (cand->num_convs > 0);
> > tree
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 11/11/23 05:43, waffl3x wrote:
[combined reply to all three threads]
On 11/9/23 23:24, waffl3x wrote:
There are a few known issues still present in this patch. Most importantly,
the implicit object argument fails to convert when passed to by-value xobj
parameters. This occurs both for xobj parameters that match the argument type
and xobj parameters that are unrelated to the object type, but have valid
conversions available. This behavior can be observed in the
explicit-obj-by-value[1-3].C tests. The implicit object argument appears to be
simply reinterpreted instead of any conversion applied. This is elaborated on
in the test cases.
Yes, that's because of:
@@ -9949,7 +9951,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_XOBJ_MEMBER_FUNC_P (fn))
We don't want to take this path for xob fns. Instead I think we need to
change the existing:
gcc_assert (first_arg == NULL_TREE);
to assert that if first_arg is non-null, we're dealing with an xob fn,
and then go ahead and do the same conversion as the loop body on first_arg.
Despite this, calls where there is no valid conversion
available are correctly rejected, which I find surprising. The
explicit-obj-by-value4.C testcase demonstrates this odd but correct behavior.
Yes, because checking for conversions is handled elsewhere.
Yeah, as I noted above I realized that just handling it the same way as
iobj member functions is fundamentally broken. I was staring at it last
night and eventually realized that I could just copy the loop body. I
ended up asserting in the body handling the implicit object argument
for xobj member functions that first_arg != NULL_TREE, which I wasn't
sure of, but it seems to work.
That sounds like it might cause trouble with
struct A {
void f(this A);
};
int main()
{
(::f) (A());
}
I will check to see what the behavior with this is. This sounds related
to the next question I asked as well.
I tried asking in IRC if there are any circumstances where first_arg
would be null for a non-static member function and I didn't get an
answer. The code above seemed to indicate that it could be. It just
looks like old code that is no longer valid and never got removed.
Consequently this function has made it on my list of things to refactor
:^).
Right, first_arg is only actually used for the implicit object argument,
it's just easier to store it separately from the arguments in (). I'm
not sure which code you mean is no longer valid?
Yeah I agree that it's easier to store it separately.
-- call.cc:build_over_call
```
else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
{
tree arg = build_this (first_arg != NULL_TREE
? first_arg
: (*args)[arg_index]);
```
The trouble is, the code (shown above) does not assume that this holds
true. It handles the case where the implicit object argument was passed
in with the rest of the arguments. As far as I've observed, it seems
like it's always passed in through the first_arg member of cand, which
is what I was referring to here.
ended up asserting in the body handling the implicit object argument
for xobj member functions that first_arg != NULL_TREE, which I wasn't
sure of, but it seems to work.
Since it wasn't clear what I was referring to, here is the code that I
wrote (copied from the loop really) handling the case. In case it isn't
obvious, I didn't snip the code in the METHOD_TYPE block, it's just
snipped here as it's not code I've modified. I'm hopeful that the case
you mentioned above is not problematic, but like I said I will be sure
to test it.
-- call.cc:build_over_call
```
else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
{
/* SNIP */
if (first_arg != NULL_TREE)
first_arg = NULL_TREE;
else
++arg_index;
++i;
is_method = 1;
}
else if (DECL_XOBJ_MEMBER_FUNC_P (fn))
{
gcc_assert (cand->first_arg);
gcc_assert (cand->num_convs > 0);
tree type = TREE_VALUE (parm);
tree arg = cand->first_arg;
bool conversion_warning = true;
conv = convs[0];
/* Set user_conv_p on the argument conversions, so rvalue/base handling
knows not to allow any more UDCs. This needs to happen after we
process cand->warnings. */
if (flags & LOOKUP_NO_CONVERSION)
conv->user_conv_p = true;
tsubst_flags_t arg_complain = complain;
if (!conversion_warning)
arg_complain &= ~tf_warning;
if (arg_complain & tf_warning)
maybe_warn_pessimizing_move (arg, type, /*return_p*/false);
val = convert_like_with_context (conv, arg, fn, 0,
arg_complain);
val = convert_for_arg_passing (type, val,
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
Just a quick addition here as I was starting to work on things I
realized where some misunderstandings were coming from. (Please also
see my previous e-mail, it is all still relevant, I just wanted to
clarify this.)
(From the other thread)
> > @@ -9949,7 +9951,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_XOBJ_MEMBER_FUNC_P (fn))
>
>
> We don't want to take this path for xob fns. Instead I think we need to
> change the existing:
>
> > gcc_assert (first_arg == NULL_TREE);
>
>
> to assert that if first_arg is non-null, we're dealing with an xob fn,
> and then go ahead and do the same conversion as the loop body on first_arg.
(This thread)
> > Yeah, as I noted above I realized that just handling it the same way as
> > iobj member functions is fundamentally broken. I was staring at it last
> > night and eventually realized that I could just copy the loop body. I
> > ended up asserting in the body handling the implicit object argument
> > for xobj member functions that first_arg != NULL_TREE, which I wasn't
> > sure of, but it seems to work.
>
>
> That sounds like it might cause trouble with
>
> struct A {
> void f(this A);
> };
>
> int main()
> {
> (::f) (A());
> }
>
Upon reviewing your reply in the other thread, I noticed you are
maybe misunderstanding the assertion a little bit.
```
else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
{
/* SNIP */
if (first_arg != NULL_TREE)
first_arg = NULL_TREE;
else
++arg_index;
++i;
is_method = 1;
}
else if (DECL_XOBJ_MEMBER_FUNC_P (fn))
{
gcc_assert (cand->first_arg);
/* SNIP */
first_arg = NULL_TREE;
}
```
I already showed my code here but I didn't think to include the
previous conditional block to demonstrate how the first_arg variable
gets handled in it. Although, maybe you understood they set it to
NULL_TREE by the end of the block and I just poorly communicated that I
would be doing the same.
Perhaps you think it better to handle the implicit object argument
within the loop, but given that it is stored in first_arg I don't think
that would be correct. Granted, as I previously said, maybe there are
some situations where it isn't that I haven't encountered yet. The
other thing is there is some handling in the loop that isn't relevant
to the implicit object argument. Well, I would also just argue this
function needs a fair degree of refactoring, but I've already talked
about that.
Anyway, hopefully that clarifies things, and hopefully things actually
needed to be clarified and I'm not being a fool here. :) That's all I
had to add about this, back to work now.
Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> [combined reply to all three threads] > > On 11/9/23 23:24, waffl3x wrote: > > > > > 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 == ptrmemfunc_vbit_in_pfn) > > > > ? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)` > > > > > > So yes, it was for PMFs using the low bit of the pointer to indicate a > > > virtual member function. Since an xob memfn can't be virtual, it's > > > correct for them to have the same alignment as a static memfn. > > > > Is it worth considering whether we want to support virtual xobj member > > functions in the future? If that were the case would it be better if we > > aligned things a little differently here? Or might it be better if we > > wanted to support it as an extension to just effectively translate the > > declaration back to one that is a METHOD_TYPE? I imagine this would be > > the best solution for non-standard support of the syntax. We would > > simply have to forbid by-value and conversion semantics and on the > > user's side they would get consistent syntax. > > > > However, this flies in the face of the defective/contradictory spec for > > virtual function overrides. So I'm not really sure whether we would > > want to do this. I just want to raise the question before we lock in > > the alignment, if pushing the patch locks it in that is, I'm not really > > sure if it needs to be stable or not. > > > It doesn't need to be stable; we can increase the alignment of decls as > needed in new code without breaking older code. Okay great, good to know, it seems so obvious when you put it that way. > > > > 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. > > > > > > That doesn't sound like a problem, but I'm curious about the specific > > > output you're seeing. > > > > I've attached a few test result comparisons so you can take a look. > > > Looks like you're comparing results from different build directories and > the libitm test wrongly includes the build directory in the test "name". > So yeah, just noise. AH okay that makes sense. > > Side note, would you prefer I compile the lambda and by-value fixes > > into a new version of this patch? Or as a separate patch? Originally I > > had planned to put it in another patch, but I identified that the code > > I wrote in build_over_call was kind of fundamentally broken and it was > > almost merely coincidence that it worked at all. In light of this and > > your comments (which I've skimmed, I will respond directly below) I > > think I should just revise this patch with everything else. > > > Agreed. Will do then. > > > > There are a few known issues still present in this patch. Most > > > > importantly, > > > > the implicit object argument fails to convert when passed to by-value > > > > xobj > > > > parameters. This occurs both for xobj parameters that match the > > > > argument type > > > > and xobj parameters that are unrelated to the object type, but have > > > > valid > > > > conversions available. This behavior can be observed in the > > > > explicit-obj-by-value[1-3].C tests. The implicit object argument > > > > appears to be > > > > simply reinterpreted instead of any conversion applied. This is > > > > elaborated on > > > > in the test cases. > > > > > > Yes, that's because of: > > > > > > > @@ -9949,7 +9951,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_XOBJ_MEMBER_FUNC_P (fn)) > > > > > > We don't want to take this path for xob fns. Instead I think we need to > > > change the existing: > > > > > > > gcc_assert (first_arg == NULL_TREE); > > > > > > to assert that if first_arg is non-null, we're dealing with an xob fn, > > > and then go ahead and do the same conversion as the loop body on > > > first_arg. > > > > > > > Despite this, calls where there is no valid conversion > > > > available are correctly rejected, which I find surprising. The > > > > explicit-obj-by-value4.C testcase demonstrates this odd but correct > > > > behavior. > > > > > > Yes, because checking for conversions is handled elsewhere. > > > > Yeah, as I noted above I realized that just handling it the same
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
[combined reply to all three threads]
On 11/9/23 23:24, waffl3x wrote:
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 == ptrmemfunc_vbit_in_pfn) \\ ? MAX
(FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)`
So yes, it was for PMFs using the low bit of the pointer to indicate a
virtual member function. Since an xob memfn can't be virtual, it's
correct for them to have the same alignment as a static memfn.
Is it worth considering whether we want to support virtual xobj member
functions in the future? If that were the case would it be better if we
aligned things a little differently here? Or might it be better if we
wanted to support it as an extension to just effectively translate the
declaration back to one that is a METHOD_TYPE? I imagine this would be
the best solution for non-standard support of the syntax. We would
simply have to forbid by-value and conversion semantics and on the
user's side they would get consistent syntax.
However, this flies in the face of the defective/contradictory spec for
virtual function overrides. So I'm not really sure whether we would
want to do this. I just want to raise the question before we lock in
the alignment, if pushing the patch locks it in that is, I'm not really
sure if it needs to be stable or not.
It doesn't need to be stable; we can increase the alignment of decls as
needed in new code without breaking older code.
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.
That doesn't sound like a problem, but I'm curious about the specific
output you're seeing.
I've attached a few test result comparisons so you can take a look.
Looks like you're comparing results from different build directories and
the libitm test wrongly includes the build directory in the test "name".
So yeah, just noise.
Side note, would you prefer I compile the lambda and by-value fixes
into a new version of this patch? Or as a separate patch? Originally I
had planned to put it in another patch, but I identified that the code
I wrote in build_over_call was kind of fundamentally broken and it was
almost merely coincidence that it worked at all. In light of this and
your comments (which I've skimmed, I will respond directly below) I
think I should just revise this patch with everything else.
Agreed.
There are a few known issues still present in this patch. Most importantly,
the implicit object argument fails to convert when passed to by-value xobj
parameters. This occurs both for xobj parameters that match the argument type
and xobj parameters that are unrelated to the object type, but have valid
conversions available. This behavior can be observed in the
explicit-obj-by-value[1-3].C tests. The implicit object argument appears to be
simply reinterpreted instead of any conversion applied. This is elaborated on
in the test cases.
Yes, that's because of:
@@ -9949,7 +9951,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_XOBJ_MEMBER_FUNC_P (fn))
We don't want to take this path for xob fns. Instead I think we need to
change the existing:
gcc_assert (first_arg == NULL_TREE);
to assert that if first_arg is non-null, we're dealing with an xob fn,
and then go ahead and do the same conversion as the loop body on first_arg.
Despite this, calls where there is no valid conversion
available are correctly rejected, which I find surprising. The
explicit-obj-by-value4.C testcase demonstrates this odd but correct behavior.
Yes, because checking for conversions is handled elsewhere.
Yeah, as I noted above I realized that just handling it the same way as
iobj member functions is fundamentally broken. I was staring at it last
night and eventually realized that I could just copy the loop body. I
ended up asserting in the body handling the implicit object argument
for xobj member functions that first_arg != NULL_TREE, which I wasn't
sure of, but it seems to work.
That sounds like it might cause trouble with
struct A {
void f(this A);
};
int main()
{
(::f) (A());
}
I tried asking in IRC if there are any circumstances where first_arg
would be null for a non-static member function and I didn't get an
answer. The code above seemed to indicate that it could be. It just
looks like old code that is no longer valid
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> > 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 == ptrmemfunc_vbit_in_pfn) \\ ? MAX
> > (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)`
>
>
> So yes, it was for PMFs using the low bit of the pointer to indicate a
> virtual member function. Since an xob memfn can't be virtual, it's
> correct for them to have the same alignment as a static memfn.
Is it worth considering whether we want to support virtual xobj member
functions in the future? If that were the case would it be better if we
aligned things a little differently here? Or might it be better if we
wanted to support it as an extension to just effectively translate the
declaration back to one that is a METHOD_TYPE? I imagine this would be
the best solution for non-standard support of the syntax. We would
simply have to forbid by-value and conversion semantics and on the
user's side they would get consistent syntax.
However, this flies in the face of the defective/contradictory spec for
virtual function overrides. So I'm not really sure whether we would
want to do this. I just want to raise the question before we lock in
the alignment, if pushing the patch locks it in that is, I'm not really
sure if it needs to be stable or not.
> > 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.
> >
> > :10331~decl.cc grokfndecl
> > `int staticp = ctype && TREE_CODE (type) == FUNCTION_TYPE;`
> > :10506~decl.cc 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) = ctype; else DECL_CONTEXT (decl) = FROB_CONTEXT
> > (current_decl_namespace ());`
> > And just a few lines down;
> > :10529~decl.cc
> > `/* Should probably propagate const out from type to decl I bet (mrs). */
> > if (staticp) { DECL_STATIC_FUNCTION_P (decl) = 1; DECL_CONTEXT (decl) =
> > 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 f3665bdc1799c0421490b5e655f977570354
> > Author: Nathan Sidwell nat...@acm.org
> > Date: Tue Jul 28 08:57:36 2020 -0700
> >
> > c++: Set more DECL_CONTEXTs
> >
> > 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.
> >
> > 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.
>
>
> Removing the redundant DECL_CONTEXT setting seems appropriate, and
> changing how staticp is handled to reflect that xobfns can also have
> FUNCTION_TYPE.
I removed static_p as it was only used in that one case, I'm pretty
happy with the resulting code but I saw you replied on the patch as
well so I'll see if you commented on it in the review and address your
thoughts there.
> > All tests
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 11/4/23 02:40, waffl3x wrote:
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 == ptrmemfunc_vbit_in_pfn) \
? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)
```
So yes, it was for PMFs using the low bit of the pointer to indicate a
virtual member function. Since an xob memfn can't be virtual, it's
correct for them to have the same alignment as a static memfn.
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 = ctype && TREE_CODE (type) == 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) = ctype;
else
DECL_CONTEXT (decl) = 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) = 1;
DECL_CONTEXT (decl) = 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 f3665bdc1799c0421490b5e655f977570354
Author: Nathan Sidwell
Date: Tue Jul 28 08:57:36 2020 -0700
c++: Set more DECL_CONTEXTs
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.
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.
Removing the redundant DECL_CONTEXT setting seems appropriate, and
changing how staticp is handled to reflect that xobfns can also have
FUNCTION_TYPE.
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.
That doesn't sound like a problem, but I'm curious about the specific
output you're seeing.
Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
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 == ptrmemfunc_vbit_in_pfn) \
? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)
```
--decl.cc:10400 grokfndecl
```
if (TREE_CODE (type) == METHOD_TYPE)
{
tree parm = build_this_parm (decl, type, quals);
DECL_CHAIN (parm) = parms;
parms = 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 = ctype && TREE_CODE (type) == 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) = ctype;
else
DECL_CONTEXT (decl) = 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) = 1;
DECL_CONTEXT (decl) = 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 f3665bdc1799c0421490b5e655f977570354
Author: Nathan Sidwell
Date: Tue Jul 28 08:57:36 2020 -0700
c++: Set more DECL_CONTEXTs
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.
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.
>
>
> 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.
>
>
> 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
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 11/3/23 00:44, waffl3x wrote: That leaves 2, 4, and 5. 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? 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) TREE_CHECK2 (T, FUNCTION_TYPE, METHOD_TYPE) -- gcc/tree.h:2518 #define TYPE_METHOD_BASETYPE(NODE) \ (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. 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. 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 would certainly expect it to be getting set already. 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. 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'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. Good plan. :) 4. I have no comment here, but it does concern me since I don't understand it at all. 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? 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. 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. Yes, "pointer_to_this" is just a cache of the type that is a pointer to the type you're looking at. You are correct that it has nothing to do with the C++ 'this'. The baselink binfo field has the private flag set for xobj member functions, iobj member functions does not. 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
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> > That leaves 2, 4, and 5. > > > > 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? > > > 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) TREE_CHECK2 (T, FUNCTION_TYPE, METHOD_TYPE) -- gcc/tree.h:2518 #define TYPE_METHOD_BASETYPE(NODE) \ (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. > > > 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? > > > 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. > > > 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. > > > 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
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 10/28/23 00:07, waffl3x wrote:
I wanted to change DECL_NONSTATIC_MEMBER_FUNCTION_P to include explicit
object member functions, but it had some problems when I made the
modification. I also noticed that it's used in cp-objcp-common.cc so
would making changes to it be a bad idea?
-- cp-tree.h
```
/* Nonzero for FUNCTION_DECL means that this decl is a non-static
member function. */
#define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
(TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
```
I didn't want to investigate the problems as I was knee deep in
investigating the addressof bug. So I instead modified
DECL_FUNCTION_MEMBER_P to include explicit object member functions and
moved on.
-- cp-tree.h
```
/* Nonzero for FUNCTION_DECL means that this decl is a member function
(static or non-static). */
#define DECL_FUNCTION_MEMBER_P(NODE) \
(DECL_NONSTATIC_MEMBER_FUNCTION_P (NODE) || DECL_STATIC_FUNCTION_P (NODE) \
|| DECL_IS_XOBJ_MEMBER_FUNC (NODE))
```
I am mostly just mentioning this here in case it becomes more relevant
later. Looking at how much DECL_NONSTATIC_MEMBER_FUNCTION_P is used
throughout the code I now suspect that adding explicit object member
functions to it might cause xobj member functions to be treated as
regular member functions when they should not be.
If this change were to stick it would cause a discrepancy in the
behavior of DECL_NONSTATIC_MEMBER_FUNCTION_P and it's name. If we were
to do this, I think it's important we document the discrepancy and why
it exists, and in the future, it should possibly be refactored. One
option would be to simply rename it to DECL_IOBJ_MEMBER_FUNCTION_P.
After all, I suspect that it's unlikely that the current macro
(DECL_NONSTATIC_MEMBER_FUNCTION_P) is being used in places that concern
explicit object member functions. So just adding explicit object member
functions to it will most likely just result in headaches.
It seems to me that would be the best solution, so when and if it comes
up again, I think that route should be considered.
Agreed, it sounds good to rename the current macro and then add a new
macro that includes both implicit and explicit, assuming that's a useful
category.
Secondly, there are some differences in the nodes describing an
explicit object member function from those describing static member
functions and implicit object member functions that I am not sure
should be present.
I did my best to summarize the differences, if you want the logs of
tree_debug that I derived them from I can provide them. Most of my
understanding of the layout of the nodes is from reading print-tree.cc
and looking at debug_tree outputs, so it's possible I made a mistake.
I am opting to use the names of members as they are output by
debug_tree, I recognize this is not always the actual name of the
member in the actual tree_node structures.
Additionally, some of the differences listed are to be expected and are
most likely the correct values for each node. However, I wanted to be
exhaustive when listing them just in case I am mistaken in my opinion
on whether the differences should or should not occur.
The following declarations were used as input to the compiler.
iobj decl:
struct S { void f() {} };
xobj decl:
struct S { void f(this S&) {} };
static decl:
struct S { static void f(S&) {} };
These differences can be observed in the return values of
grokdeclarator for each declaration.
1. function_decl::type::tree_code
iobj: method_type
xobj: function_type
stat: function_type
2. function_decl::type::method basetype
iobj:
xobj: NULL/no output
stat: NULL/no output
3. function_decl::type::arg-types::tree_list[0]::value
iobj:
xobj:
stat:
4. function_decl::decl_6
iobj: false/no output
xobj: false/no output
stat: true
5. function_decl::align
iobj: 16
xobj: 8
stat: 8
6. function_decl::result::uid
iobj: D.2513
xobj: D.2513
stat: D.2512
7. function_decl::full-name
iobj: "void S::f()"
xobj: "void S::f(this S&)"
Differences 1, 3, and 7 seem obviously correct to me for all 3
declarations, 6 is a little bizarre to me, but since it's just a UID
it's merely an oddity, I doubt it is concerning.
Agreed.
That leaves 2, 4, and 5.
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?
method basetype is only for METHOD_TYPE; if you want the containing type
of the function, that's DECL_CONTEXT.
4. I have no comment here, but it does concern me since I don't
understand it at all.
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.
5. I am
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Wed, Nov 01, 2023 at 11:15:56PM +, waffl3x wrote: > Just want to quickly check, when is the cutoff for GCC14 exactly? I Nov 18th EOD. But generally it is cutoff for posting new feature patches that could be accepted, patches posted before the deadline if reviewed soon after the deadline still can make it in. Jakub
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
The problem with operators is fixed, now starts a long period of testing. It's been so long since I've gotten to this part that I almost forgot that I have to do it :'). When/if regtests and bootstrap all pass I will format the patch and submit it. --- Original Message --- On Wednesday, November 1st, 2023 at 5:15 PM, waffl3x wrote: > > > Just want to quickly check, when is the cutoff for GCC14 exactly? I > want to know how much time I have left to try to tackle this bug with > subscript. I'm going to be crunching out final stuff this week and I'll > try to get a (probably non-final) patch for you to review today. > > Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
Just want to quickly check, when is the cutoff for GCC14 exactly? I want to know how much time I have left to try to tackle this bug with subscript. I'm going to be crunching out final stuff this week and I'll try to get a (probably non-final) patch for you to review today. Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
I woke up today and figured it out in about 30 minutes, I don't know if
this solution would be okay but I am running away from
cp_build_addr_expr_1 for now. I think this is another place I will have
the urge to refactor in the future, but if I keep looking at it right
now I am just going to waste all my energy before I finish everything
else.
This line
```
else if (BASELINK_P (TREE_OPERAND (arg, 1)))
```
near the bottom of the function is almost certainly dead. The switch
case (near the top) for baselink reassigns arg.
```
case BASELINK:
arg = BASELINK_FUNCTIONS (arg);
```
And I'm pretty sure a baselink's functions can't be a baselink, so that
case near the bottom is almost certainly dead. I've put it in my todo
so I will look at it in the future.
Anyway, here is my new solution, it seems to work, but as indicated in
the comment (for myself) I'm not sure this handles constexpr things
right as near the bottom of this function there is some constexpr
handling. I know I said it took me 30 minutes but I realized I had some
holes I needed to check as I was writing this email, so it was more
like 2 hours.
```
/* Forming a pointer-to-member is a use of non-pure-virtual fns. */
if (TREE_CODE (t) == FUNCTION_DECL
&& !DECL_PURE_VIRTUAL_P (t)
&& !mark_used (t, complain) && !(complain & tf_error))
return error_mark_node;
/* Exception for non-overloaded explicit object member function. */
/* I am pretty sure this is still not perfect, I think we aren't
handling some constexpr stuff, but I am leaving it for now. */
if (TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE)
return build_address (t);
type = build_ptrmem_type (context_for_name_lookup (t),
TREE_TYPE (t));
t = make_ptrmem_cst (type, t);
return t;
}
```
I'm sharing it because I am still uncertain on whether this is the
ideal solution, this function is kind of a mess in general (sorry) so
it's hard to tell.
There's still a few things in my previous email that I would like
looked at, primarily the differences I observed in returns from
grokdeclarator, but I am glad to finally be moving forward on to other
things. Hopefully I can put together the patches with this one fixed.
(Of course I still have to fix the other bug I found as well.)
I had thought I might try to get lambda support and by value xobj
params working in this patch, but I think that will still be something
I work on once I am finished the initial patch. I still want to get
both of those into GCC14 though so I probably need to speed things up.
Alex
>
>
> I've been under the weather so I took a few days break, I honestly was
> also very reluctant to pick it back up. The current problem is what I
> like to call "not friendly", but I am back at it now.
>
> > > I don't understand what this means exactly, under what circumstances
> > > would find the member function. Oh, I guess while in the body of
> > > it's class, I hadn't considered that. Is that what you're referring to?
> >
> > Right:
> >
> > struct A {
> > void g(this A&);
> > A() {
> > ::g; // ok
> > // same error as for an implicit object member function
> > }
> > };
>
>
> I fully get this now, I threw together a test for it so this case
> doesn't get forgotten about. Unfortunately though, I am concerned that
> the approach I was going to take to fix the crash would have the
> incorrect behavior for this.
>
> Here is what I added to cp_build_addr_expr_1 with context included.
> `case OFFSET_REF: offset_ref: /* Turn a reference to a non-static data member
> into a pointer-to-member. */ { tree type; tree t; gcc_assert (PTRMEM_OK_P
> (arg)); t = TREE_OPERAND (arg, 1); if (TYPE_REF_P (TREE_TYPE (t))) { if
> (complain & tf_error) error_at (loc, "cannot create pointer to reference
> member %qD", t); return error_mark_node; } /* -- Waffl3x additions start --
> */ /* Exception for non-overloaded explicit object member function. */ if
> (TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE) return build1 (ADDR_EXPR,
> unknown_type_node, arg); /* -- Waffl3x additions end -- */ /* Forming a
> pointer-to-member is a use of non-pure-virtual fns. */ if (TREE_CODE (t) ==
> FUNCTION_DECL && !DECL_PURE_VIRTUAL_P (t) && !mark_used (t, complain) &&
> !(complain & tf_error)) return error_mark_node;`
>
> I had hoped this naive solution would work just fine, but unfortunately
> the following code fails to compile with an error.
>
> `struct S { void f(this S&) {} }; int main() { void (*a)(S&) = ::f; }`
> normal_static.C: In function ‘int main()’:
> normal_static.C:13:25: error: cannot convert ‘S::f’ from type ‘void(S&)’ to
> type ‘void (*)(S&)’
> 13 | void (*a)(S&) = ::f;
> | ^
>
> So clearly it isn't going to be that easy. I went up and down looking
> at how the single static case is handled, and tried to read the code in
> build_ptrmem_type and build_ptrmemfunc_type but I had a hard time
>
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
I've been under the weather so I took a few days break, I honestly was
also very reluctant to pick it back up. The current problem is what I
like to call "not friendly", but I am back at it now.
> > I don't understand what this means exactly, under what circumstances
> > would find the member function. Oh, I guess while in the body of
> > it's class, I hadn't considered that. Is that what you're referring to?
>
>
> Right:
>
> struct A {
> void g(this A&);
> A() {
> ::g; // ok
> // same error as for an implicit object member function
> }
> };
I fully get this now, I threw together a test for it so this case
doesn't get forgotten about. Unfortunately though, I am concerned that
the approach I was going to take to fix the crash would have the
incorrect behavior for this.
Here is what I added to cp_build_addr_expr_1 with context included.
```
case OFFSET_REF:
offset_ref:
/* Turn a reference to a non-static data member into a
pointer-to-member. */
{
tree type;
tree t;
gcc_assert (PTRMEM_OK_P (arg));
t = TREE_OPERAND (arg, 1);
if (TYPE_REF_P (TREE_TYPE (t)))
{
if (complain & tf_error)
error_at (loc,
"cannot create pointer to reference member %qD", t);
return error_mark_node;
}
/* -- Waffl3x additions start -- */
/* Exception for non-overloaded explicit object member function. */
if (TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE)
return build1 (ADDR_EXPR, unknown_type_node, arg);
/* -- Waffl3x additions end -- */
/* Forming a pointer-to-member is a use of non-pure-virtual fns. */
if (TREE_CODE (t) == FUNCTION_DECL
&& !DECL_PURE_VIRTUAL_P (t)
&& !mark_used (t, complain) && !(complain & tf_error))
return error_mark_node;
```
I had hoped this naive solution would work just fine, but unfortunately
the following code fails to compile with an error.
```
struct S {
void f(this S&) {}
};
int main() {
void (*a)(S&) = ::f;
}
```
normal_static.C: In function ‘int main()’:
normal_static.C:13:25: error: cannot convert ‘S::f’ from type ‘void(S&)’ to
type ‘void (*)(S&)’
13 | void (*a)(S&) = ::f;
| ^
So clearly it isn't going to be that easy. I went up and down looking
at how the single static case is handled, and tried to read the code in
build_ptrmem_type and build_ptrmemfunc_type but I had a hard time
figuring it out.
The good news is, this problem was difficult enough that it made me
pick a proper diff tool with regex support instead of using a silly web
browser tool and pasting things into it. Or worse, pasting them into a
tool and doing replacement and then pasting them into the silly web
browser tool. I have been forced to improve my workflow thanks to this
head scratcher. So it's not all for naught.
Back on topic, it's not really the optimization returning a baselink
that is causing the original crash. It's just the assert in
build_ptrmem_type failing when a FUNCTION_TYPE is reaching it. The
optimization did throw me for a loop when I was comparing how my
previous version (that incorrectly set the lang_decl_fn ::
static_function flag) was handling things. Looking back, I think I
explained myself and the methodology I was using to investigate really
poorly, I apologize for the confusion I might have caused :).
To state it plainly, it seems to me that the arg parameter being passed
into cp_build_addr_expr_1 for explicit object member functions is
(mostly) pretty much correct and what we would want.
So the whole thing with the baselink optimization was really just a red
herring that I was following. Now that I have a better understanding of
what's happening leading up to and in cp_build_addr_expr_1 I don't
think it's relevant at all for this problem. With that said, I am
questioning again if the optimization that returns a baselink node is
the right way to do things. So this is something I'm going to put down
into my little notes text file to investigate at a later time, and
forget about it for the moment as it shouldn't be causing any friction
for us here.
Anyway, as I eluded to above, if I can't figure out the right way to
solve this problem in a decent amount of time I think I'm going to
leave it for now. I'll come back to it once other higher priority
things are fixed or finished. And hopefully someone more familiar with
this area of the code will have a better idea of what we need to do to
handle this case in a non-intrusive manner.
That wraps up my current status on this specifically. But while
investigating it I uncovered a few things that I feel are important to
discuss/note.
I wanted to change DECL_NONSTATIC_MEMBER_FUNCTION_P to include explicit
object member functions, but it had some problems when I made the
modification. I also noticed that it's used in cp-objcp-common.cc so
would making changes to it be a bad
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 10/20/23 00:34, waffl3x wrote:
Based on what you've said, I assume that OFFSET_REF handles static
member functions that are overloaded. But as I've said this seems to
contradict the comments I'm reading, so I'm not sure that I'm
understanding you correctly.
That's right. For instance,
struct A {
static void g();
static void g(int);
};
void (*p)(int) = ::g; // cp_build_addr_expr_1 gets an OFFSET_REF
I think we need the OFFSET_REF for an explicit-object member function
because it expresses that the code satisfies the requirement "If the
operand names an explicit object member function, the operand shall be a
qualified-id."
I do agree here, but it does reinforce that OFFSET_REF is no longer
just for members represented by pointer to member type. So that might
be something to take into consideration.
An OFFSET_REF that isn't type_unknown_p, agreed.
It might simplify things to remove the optimization in build_offset_ref
so we get an OFFSET_REF even for a single static member function, and
add support for that to cp_build_addr_expr_1.
I don't think this should be necessary, the "right thing" should just
be done for explicit-object member functions. With all the stuff going
on here that I missed I'm starting to wonder how function overloads
ever worked at all in my patch. On the other hand though, this
optimization probably could be documented better, but I very well might
have missed it even if it were.
Hell, maybe it needs a greater redesign altogether, it seems strange to
me to bundle overload information in with a construct for a specific
expression. (Assuming that's whats happening of course, I still don't
fully understand it.) It's not like this has rules unique to it for how
overload resolution is decided, right? Initializing a param/variable of
pointer to function type with an overloaded function resolves that with
similar rules, I think? Maybe it is a little different now that I write
it out loud.
I wasn't going to finish my musings about that, but it made me realize
that it might not actually be correct for address of explicit-object
member functions to be wrapped by OFFSET_REF. I mean surely it's fine
because based on what you've said static member functions are also
wrapped by OFFSET_REF, so it's likely fully implemented, especially
considering things worked before. But now that there are 2 different
varieties of class members that the address of them can be taken, it
might make sense to split things up a bit? Then again, why were static
member functions ever handled the same way? Taking the address of other
static members isn't handled in the same way here is it?
Functions are different because of overloading; in general we can't
decide what an expression that names a function actually means until we
have enough context to decide which function, exactly. So we represent
the id-expression largely as lookup+syntax until overload resolution
turns it into a specific function. The type_unknown_p check earlier in
cp_build_addr_expr_1 is for that case.
Yeah this all makes sense, but that's why I was confused by the
following documentation from cp-tree.def.
```
/* An OFFSET_REF is used in two situations:
1. An expression of the form `A::m' where `A' is a class and `m' is
a non-static member. In this case, operand 0 will be a TYPE
(corresponding to `A') and operand 1 will be a FIELD_DECL,
BASELINK, or TEMPLATE_ID_EXPR (corresponding to `m').
The expression is a pointer-to-member if its address is taken,
but simply denotes a member of the object if its address is not
taken.
```
Yeah, I'll adjust that statement to be more conditional. Is this clearer?
1. An expression of the form `A::m' where `A' is a class and `m' is
a non-static member or an overload set. In this case, operand 0
will be a TYPE (corresponding to `A') and operand 1 will be a
FIELD_DECL, BASELINK, or TEMPLATE_ID_EXPR (corresponding to
`m').
The expression is a pointer-to-member if its address is taken
(and
if, after any overload resolution, 'm' does not designate a
static or explicit object member function). It simply denotes a
member of the object if its address is not taken.
An OFFSET_REF that isn't type_unknown_p, agreed.
But I suppose that's what this might have been referring to.
So is that the case then? OFFSET_REF might be for a regular address of
member expression unless it's type_unknown_p?
If it's type_unknown_p we don't know which member it is, so we don't
know whether taking its address gives a PMF or a regular pointer.
If it's not type_unknown_p, we know which member it is, and we currently
skip building it at all for static members, so taking its address always
gives a pointer to member. explicit object member functions will make
that assumption no longer valid.
An id-expression that names a single non-template function
(!really_overloaded_fn) is handled
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> >
> > Based on what you've said, I assume that OFFSET_REF handles static
> > member functions that are overloaded. But as I've said this seems to
> > contradict the comments I'm reading, so I'm not sure that I'm
> > understanding you correctly.
>
>
> That's right. For instance,
>
> struct A {
> static void g();
> static void g(int);
> };
>
> void (*p)(int) = ::g; // cp_build_addr_expr_1 gets an OFFSET_REF
>
> > > > I think we need the OFFSET_REF for an explicit-object member function
> > > > because it expresses that the code satisfies the requirement "If the
> > > > operand names an explicit object member function, the operand shall be a
> > > > qualified-id."
> >
> > I do agree here, but it does reinforce that OFFSET_REF is no longer
> > just for members represented by pointer to member type. So that might
> > be something to take into consideration.
>
>
> An OFFSET_REF that isn't type_unknown_p, agreed.
>
> > > > It might simplify things to remove the optimization in build_offset_ref
> > > > so we get an OFFSET_REF even for a single static member function, and
> > > > add support for that to cp_build_addr_expr_1.
> >
> > I don't think this should be necessary, the "right thing" should just
> > be done for explicit-object member functions. With all the stuff going
> > on here that I missed I'm starting to wonder how function overloads
> > ever worked at all in my patch. On the other hand though, this
> > optimization probably could be documented better, but I very well might
> > have missed it even if it were.
> >
> > Hell, maybe it needs a greater redesign altogether, it seems strange to
> > me to bundle overload information in with a construct for a specific
> > expression. (Assuming that's whats happening of course, I still don't
> > fully understand it.) It's not like this has rules unique to it for how
> > overload resolution is decided, right? Initializing a param/variable of
> > pointer to function type with an overloaded function resolves that with
> > similar rules, I think? Maybe it is a little different now that I write
> > it out loud.
> >
> > I wasn't going to finish my musings about that, but it made me realize
> > that it might not actually be correct for address of explicit-object
> > member functions to be wrapped by OFFSET_REF. I mean surely it's fine
> > because based on what you've said static member functions are also
> > wrapped by OFFSET_REF, so it's likely fully implemented, especially
> > considering things worked before. But now that there are 2 different
> > varieties of class members that the address of them can be taken, it
> > might make sense to split things up a bit? Then again, why were static
> > member functions ever handled the same way? Taking the address of other
> > static members isn't handled in the same way here is it?
>
>
> Functions are different because of overloading; in general we can't
> decide what an expression that names a function actually means until we
> have enough context to decide which function, exactly. So we represent
> the id-expression largely as lookup+syntax until overload resolution
> turns it into a specific function. The type_unknown_p check earlier in
> cp_build_addr_expr_1 is for that case.
Yeah this all makes sense, but that's why I was confused by the
following documentation from cp-tree.def.
```
/* An OFFSET_REF is used in two situations:
1. An expression of the form `A::m' where `A' is a class and `m' is
a non-static member. In this case, operand 0 will be a TYPE
(corresponding to `A') and operand 1 will be a FIELD_DECL,
BASELINK, or TEMPLATE_ID_EXPR (corresponding to `m').
The expression is a pointer-to-member if its address is taken,
but simply denotes a member of the object if its address is not
taken.
```
> An OFFSET_REF that isn't type_unknown_p, agreed.
But I suppose that's what this might have been referring to.
So is that the case then? OFFSET_REF might be for a regular address of
member expression unless it's type_unknown_p?
>
> An id-expression that names a single non-template function
> (!really_overloaded_fn) is handled somewhat differently, as we don't
> need to defer everything. But that means various special-case code.
>
> Currently build_offset_ref special-cases ::f for a single static
> member function, but we can't use the same special case for single
> explicit object member functions because we need to distinguish between
> ::f and somehow to check the requirement I quoted above. So it
> seems to me we'll need to add support for single explicit object member
> functions in the OFFSET_REF handling in cp_build_addr_expr_1. And I
> thought if we're doing that, perhaps we want to move the single static
> handling over there as well, but that's not necessary.
>
> Jason
I'm done for today but it does seem like that special case is what is
causing my crash. I found that it only jumps into the section that it
crashes in when there are no overloads. I'm
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 10/19/23 19:35, waffl3x wrote:
(waffl3x (me))
At a glance it seems like all I need to do then is disable the
PTRMEM_OK_P flag then.
I'm just now realizing that I'm almost certainly wrong about this. It
still needs PTRMEM_OK_P set if there are any implicit-object member
functions in the overload set. That is, if OFFSET_REF includes that
information... but it doesn't seem like it does? Reading the
information on OFFSET_REF, particularly build_offset_ref, seems to
indicate that OFFSET_REF (at least historically) was only for things
with a pointer to member type.
Or things that might end up with pointer-to-member type after overload
resolution.
An OFFSET_REF (with PTRMEM_OK_P) is used to express that we saw the
::f syntax, so we could build a pointer to member if it resolves to an
implicit-object member function.
For an overload set containing only a single static member function,
build_offset_ref doesn't bother to build an OFFSET_REF, but returns the
BASELINK itself.
Based on what you've said, I assume that OFFSET_REF handles static
member functions that are overloaded. But as I've said this seems to
contradict the comments I'm reading, so I'm not sure that I'm
understanding you correctly.
That's right. For instance,
struct A {
static void g();
static void g(int);
};
void (*p)(int) = ::g; // cp_build_addr_expr_1 gets an OFFSET_REF
I think we need the OFFSET_REF for an explicit-object member function
because it expresses that the code satisfies the requirement "If the
operand names an explicit object member function, the operand shall be a
qualified-id."
I do agree here, but it does reinforce that OFFSET_REF is no longer
just for members represented by pointer to member type. So that might
be something to take into consideration.
An OFFSET_REF that isn't type_unknown_p, agreed.
It might simplify things to remove the optimization in build_offset_ref
so we get an OFFSET_REF even for a single static member function, and
add support for that to cp_build_addr_expr_1.
I don't think this should be necessary, the "right thing" should just
be done for explicit-object member functions. With all the stuff going
on here that I missed I'm starting to wonder how function overloads
ever worked at all in my patch. On the other hand though, this
optimization probably could be documented better, but I very well might
have missed it even if it were.
Hell, maybe it needs a greater redesign altogether, it seems strange to
me to bundle overload information in with a construct for a specific
expression. (Assuming that's whats happening of course, I still don't
fully understand it.) It's not like this has rules unique to it for how
overload resolution is decided, right? Initializing a param/variable of
pointer to function type with an overloaded function resolves that with
similar rules, I think? Maybe it is a little different now that I write
it out loud.
I wasn't going to finish my musings about that, but it made me realize
that it might not actually be correct for address of explicit-object
member functions to be wrapped by OFFSET_REF. I mean surely it's fine
because based on what you've said static member functions are also
wrapped by OFFSET_REF, so it's likely fully implemented, especially
considering things worked before. But now that there are 2 different
varieties of class members that the address of them can be taken, it
might make sense to split things up a bit? Then again, why were static
member functions ever handled the same way? Taking the address of other
static members isn't handled in the same way here is it?
Functions are different because of overloading; in general we can't
decide what an expression that names a function actually means until we
have enough context to decide which function, exactly. So we represent
the id-expression largely as lookup+syntax until overload resolution
turns it into a specific function. The type_unknown_p check earlier in
cp_build_addr_expr_1 is for that case.
An id-expression that names a single non-template function
(!really_overloaded_fn) is handled somewhat differently, as we don't
need to defer everything. But that means various special-case code.
Currently build_offset_ref special-cases ::f for a single static
member function, but we can't use the same special case for single
explicit object member functions because we need to distinguish between
::f and somehow to check the requirement I quoted above. So it
seems to me we'll need to add support for single explicit object member
functions in the OFFSET_REF handling in cp_build_addr_expr_1. And I
thought if we're doing that, perhaps we want to move the single static
handling over there as well, but that's not necessary.
Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> (waffl3x (me)) > At a glance it seems like all I need to do then is disable the > PTRMEM_OK_P flag then. I'm just now realizing that I'm almost certainly wrong about this. It still needs PTRMEM_OK_P set if there are any implicit-object member functions in the overload set. That is, if OFFSET_REF includes that information... but it doesn't seem like it does? Reading the information on OFFSET_REF, particularly build_offset_ref, seems to indicate that OFFSET_REF (at least historically) was only for things with a pointer to member type. > > An OFFSET_REF (with PTRMEM_OK_P) is used to express that we saw the > > ::f syntax, so we could build a pointer to member if it resolves to an > > implicit-object member function. > > > > For an overload set containing only a single static member function, > > build_offset_ref doesn't bother to build an OFFSET_REF, but returns the > > BASELINK itself. Based on what you've said, I assume that OFFSET_REF handles static member functions that are overloaded. But as I've said this seems to contradict the comments I'm reading, so I'm not sure that I'm understanding you correctly. I suppose that will be pretty easy to test, so I'll just do that as well. > > I think we need the OFFSET_REF for an explicit-object member function > > because it expresses that the code satisfies the requirement "If the > > operand names an explicit object member function, the operand shall be a > > qualified-id." I do agree here, but it does reinforce that OFFSET_REF is no longer just for members represented by pointer to member type. So that might be something to take into consideration. > > It might simplify things to remove the optimization in build_offset_ref > > so we get an OFFSET_REF even for a single static member function, and > > add support for that to cp_build_addr_expr_1. I don't think this should be necessary, the "right thing" should just be done for explicit-object member functions. With all the stuff going on here that I missed I'm starting to wonder how function overloads ever worked at all in my patch. On the other hand though, this optimization probably could be documented better, but I very well might have missed it even if it were. Hell, maybe it needs a greater redesign altogether, it seems strange to me to bundle overload information in with a construct for a specific expression. (Assuming that's whats happening of course, I still don't fully understand it.) It's not like this has rules unique to it for how overload resolution is decided, right? Initializing a param/variable of pointer to function type with an overloaded function resolves that with similar rules, I think? Maybe it is a little different now that I write it out loud. I wasn't going to finish my musings about that, but it made me realize that it might not actually be correct for address of explicit-object member functions to be wrapped by OFFSET_REF. I mean surely it's fine because based on what you've said static member functions are also wrapped by OFFSET_REF, so it's likely fully implemented, especially considering things worked before. But now that there are 2 different varieties of class members that the address of them can be taken, it might make sense to split things up a bit? Then again, why were static member functions ever handled the same way? Taking the address of other static members isn't handled in the same way here is it? I'm probably spending too much time thinking about it when I don't fully understand how it's all being done, I'll just go back to poking around trying to figure it all out. Then I'll worry about whether thing's should be done differently or not. Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> A BASELINK expresses the result of name lookup for a member function, > since we need to pass information about the name lookup context along to > after overload resolution. > > An OFFSET_REF (with PTRMEM_OK_P) is used to express that we saw the > ::f syntax, so we could build a pointer to member if it resolves to an > implicit-object member function. > > For an overload set containing only a single static member function, > build_offset_ref doesn't bother to build an OFFSET_REF, but returns the > BASELINK itself. > > I think we need the OFFSET_REF for an explicit-object member function > because it expresses that the code satisfies the requirement "If the > operand names an explicit object member function, the operand shall be a > qualified-id." > > It might simplify things to remove the optimization in build_offset_ref > so we get an OFFSET_REF even for a single static member function, and > add support for that to cp_build_addr_expr_1. > > Jason Ah okay I think that sheds a little bit of light on things, and here I was trying not to involve overloads to make it easier for me to understand things, it seems it ended up making me miss some things. At a glance it seems like all I need to do then is disable the PTRMEM_OK_P flag then. I will try that and see how it goes, provided I can find where it's all setup. (I think I'm almost to the bottom of it, but it's tough to unravel so I'm not sure.) I'm also now realizing it's probably about time I add more tests involving overloads, because I avoided that early on and I don't think I ever got around to adding any for that. Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 10/19/23 17:05, waffl3x wrote: Also, I'm not sure what %qs is, should I be using that instead of %s for strings? The q prefix means quoted, with ' or other quotation marks, depending on the locale. On another topic, I have been trying to fix taking pointers to explicit object member functions today, as I ended up breaking that when I started setting static_function to false for them. Originally it just worked so I haven't touched any code related to this, but now that they aren't being treating like static member functions as much so a few things just broke. What I'm asking myself now is whether it would be appropriate to just opt-in to static member function behavior for this one, and I'm not sure that would be correct. So I started by checking what it did before I turned off the static_function flag. It's was being passed into cp_build_addr_expr_1 as a baselink node, while regular member functions are passed in as an offset_ref node. I then checked what the case is for static member function, and unsurprisingly those are also handled wrapped in a baselink node, but this raised some questions for me. I am now trying to figure out what exactly a baselink is, and why it's used for static member functions. My current best guess is that method_type nodes already hold the information that a baselink does, and that information is needed in general. If that is the case, it might just be correct to just do the same thing for explicit object member functions, but I wonder if there is more to it, but maybe there isn't. It worked just fine before when the static_function was still being set after all. Any insight on this is appreciated. A BASELINK expresses the result of name lookup for a member function, since we need to pass information about the name lookup context along to after overload resolution. An OFFSET_REF (with PTRMEM_OK_P) is used to express that we saw the ::f syntax, so we could build a pointer to member if it resolves to an implicit-object member function. For an overload set containing only a single static member function, build_offset_ref doesn't bother to build an OFFSET_REF, but returns the BASELINK itself. I think we need the OFFSET_REF for an explicit-object member function because it expresses that the code satisfies the requirement "If the operand names an explicit object member function, the operand shall be a qualified-id." It might simplify things to remove the optimization in build_offset_ref so we get an OFFSET_REF even for a single static member function, and add support for that to cp_build_addr_expr_1. Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Thu, Oct 19, 2023 at 09:31:06PM +, waffl3x wrote:
> Ah alright, I see what you're saying, I see what the difference is now.
> It's a shame we can't have the translated string insert a %s and format
> into that :^). Ah well, I guess this code is just doomed to look poor
> then, what can you do.
Consider e.g. de.po:
msgid "Generate code to check exception specifications."
msgstr "Code zur Überprüfung von Exception-Spezifikationen erzeugen."
If you try to construct the above from 2 parts as
"%s to check exception specifications.", _("Generate code")
then the german translator will need to give up, because the verb
needs to go last and noun first, so translating "Generate code"
to "Code erzeugen" and the rest can't be done, you want to put one word
in one place and another at another. A lot of languages
have quite strict rules on order of words in sentence, unlike say
Latin with its comparatively free word order.
You could be lucky, but without knowing at least all the currently
supported languages it can be hard to prove it is ok.
Furthermore, it could prevent some other language translations from being
added.
Jakub
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> No, that wouldn't be appropriate for translation.
> None of non-member, static member and explicit object member are
> something that should be printed verbatim untranslated.
> "%s function %qD cannot have cv-qualifier", _("non-member")
> etc. is still inappropriate, some language might need to reorder
> the words in one case and not another one etc.
>
> What is ok if that %s (but in that case better %qs) is say some
> language keyword which shouldn't be translated.
>
> "%qs keyword not expected"
> with
> the arg being "inline", "constexpr", "consteval" for example would
> be fine.
>
> Jakub
Ah alright, I see what you're saying, I see what the difference is now.
It's a shame we can't have the translated string insert a %s and format
into that :^). Ah well, I guess this code is just doomed to look poor
then, what can you do.
This is pretty much why I've been afraid to touch anything with these
macros, if I don't understand exactly how it works anything slightly
weird solution I use might just not work.
Anyway, I think that clears up everything regarding that now, thanks.
Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Thu, Oct 19, 2023 at 09:05:38PM +, waffl3x wrote:
> Okay so taking what you guys are saying here it sounds like it would be
> appropriate to refactor the code I was reluctant to refactor. The code
> (in grokfndecl) conditionally selects one of the two (now three with my
> changes) following strings despite them being mostly identical.
>
> "non-member function %qD cannot have cv-qualifier"
> "static member function %qD cannot have cv-qualifier"
> "explicit object member function %qD cannot have cv-qualifier"
>
> >From what I'm getting from what you two have said is that it would be
> reasonable to instead construct a string from it's two parts, just
> selecting the differing part dynamically.
>
> "%s function %qD cannot have cv-qualifier"
No, that wouldn't be appropriate for translation.
None of non-member, static member and explicit object member are
something that should be printed verbatim untranslated.
"%s function %qD cannot have cv-qualifier", _("non-member")
etc. is still inappropriate, some language might need to reorder
the words in one case and not another one etc.
What is ok if that %s (but in that case better %qs) is say some
language keyword which shouldn't be translated.
"%qs keyword not expected"
with
the arg being "inline", "constexpr", "consteval" for example would
be fine.
Jakub
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> (Jakub) > There are different kinds of format strings in GCC, the most common > are the gcc-internal-format strings. If you call a function which > is expected to take such translatable format string (in particular > a function which takes a gmsgid named argument like error, error_at, > pedwarn, warning_at, ...) and pass a string literal to that function, > nothing needs to be marked in a special way, both gcc/po/exgettext > is able to collect such literals into gcc/po/gcc.pot for translations > and the function is supposed to use gettext etc. to translate it > - e.g. see diagnostic_set_info using (gmsgid) for that. > But, if there is e.g. a temporary pointer var which points to format > strings and only that is eventually passed to the diagnostic functions, > gcc/po/exgettext won't be able to collect such literals, which is where > the G() macro comes into play and one marks the string as > gcc-internal-format with it; the translation is still handled by the > diagnostic function at runtime. The N_() macro is similar but for c-format > strings instead. The _() macro both collects for translations if it is > used with string literal, and expands to gettext call to translate it at > runtime, which is something that should be avoided if something translates > it again. > (Jason) > The protocol is described in gcc/ABOUT-GCC-NLS. In general, "strings" > passed directly to a diagnostic function don't need any decoration, but > if they're assigned to a variable first, they need G_() so they're > recognized as diagnostic strings to be added to the translation table. I read this last night and decided to sleep on it hoping it would make more sense now. I was going to write about how I'm still confused but I think it just clicked for me. I somehow didn't make the connection that any kind of of expression (like a conditional) is going to require it's use. I guess I kept thinking "but they aren't being assigned to a variable" but I get it now, it's when the strings aren't passed DIRECTLY into the diagnostic function. > (Jakub) > And another i18n rule is that one shouldn't try to construct diagnostic > messages from parts of english sentences, it is fine to fill in with %s/%qs > etc. language keywords etc. but otherwise the format string should contain > the whole diagnostic line, so that translators can reorder the words etc. > (Jason) > The () macro is used for strings that are going to be passed to a %s, > but better to avoid doing that for strings that need translation. N() > is (rarely) used for strings that aren't diagnostic format strings, but > get passed to another function that passes them to _(). Okay so taking what you guys are saying here it sounds like it would be appropriate to refactor the code I was reluctant to refactor. The code (in grokfndecl) conditionally selects one of the two (now three with my changes) following strings despite them being mostly identical. "non-member function %qD cannot have cv-qualifier" "static member function %qD cannot have cv-qualifier" "explicit object member function %qD cannot have cv-qualifier" >From what I'm getting from what you two have said is that it would be reasonable to instead construct a string from it's two parts, just selecting the differing part dynamically. "%s function %qD cannot have cv-qualifier" Just to clarify, should I be marking the "non-member", "static member", "explicit object member" strings with the _ macro then? I'm just not sure since it seems like Jason is saying they shouldn't be since the string they are being formatted into is being translated afterwards. Also, I'm not sure what %qs is, should I be using that instead of %s for strings? On another topic, I have been trying to fix taking pointers to explicit object member functions today, as I ended up breaking that when I started setting static_function to false for them. Originally it just worked so I haven't touched any code related to this, but now that they aren't being treating like static member functions as much so a few things just broke. What I'm asking myself now is whether it would be appropriate to just opt-in to static member function behavior for this one, and I'm not sure that would be correct. So I started by checking what it did before I turned off the static_function flag. It's was being passed into cp_build_addr_expr_1 as a baselink node, while regular member functions are passed in as an offset_ref node. I then checked what the case is for static member function, and unsurprisingly those are also handled wrapped in a baselink node, but this raised some questions for me. I am now trying to figure out what exactly a baselink is, and why it's used for static member functions. My current best guess is that method_type nodes already hold the information that a baselink does, and that information is needed in general. If that is the case, it might just be correct to just do the same thing for explicit object member functions, but I wonder if there is more
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 10/18/23 13:28, waffl3x wrote:
I will try to get something done today, but I was struggling with
writing some of the tests, there's also a lot more of them now. I also
wrote a bunch of musings in comments that I would like feedback on.
My most concrete question is, how exactly should I be testing a
pedwarn, I want to test that I get the correct warning and error with
the separate flags, do I have to create two separate tests for each one?
Yes. I tend to use letter suffixes for tests that vary only in flags
(and expected results), e.g. feature1a.C, feature1b.C.
Will do.
Instead of OPT_Wpedantic, this should be controlled by
-Wc++23-extensions (OPT_Wc__23_extensions)
Yeah, I'll do this.
If you wanted, you could add a more specific warning option for this
(e.g. -Wc++23-explicit-this) which is also affected by
-Wc++23-extensions, but I would lean toward just using the existing
flag. Up to you.
I brought it up in irc and there was some pushback to my point of view
on it, so I'll just stick with OPT_Wc__23_extensions for now. I do
think a more sophisticated interface would be beneficial but I will
bring discussion around that up again in the future.
I've seen plenty of these G_ or _ macros on strings around like in
grokfndecl for these errors.
G_("static member function %qD cannot have cv-qualifier")
G_("non-member function %qD cannot have cv-qualifier")
G_("static member function %qD cannot have ref-qualifier")
G_("non-member function %qD cannot have ref-qualifier")
I have been able to figure out it relates to translation, but not
exactly what the protocol around them is.
The protocol is described in gcc/ABOUT-GCC-NLS. In general, "strings"
passed directly to a diagnostic function don't need any decoration, but
if they're assigned to a variable first, they need G_() so they're
recognized as diagnostic strings to be added to the translation table.
The _() macro is used for strings that are going to be passed to a %s,
but better to avoid doing that for strings that need translation. N_()
is (rarely) used for strings that aren't diagnostic format strings, but
get passed to another function that passes them to _().
Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On Wed, Oct 18, 2023 at 05:28:10PM +, waffl3x wrote:
> I've seen plenty of these G_ or _ macros on strings around like in
> grokfndecl for these errors.
>
> G_("static member function %qD cannot have cv-qualifier")
> G_("non-member function %qD cannot have cv-qualifier")
>
> G_("static member function %qD cannot have ref-qualifier")
> G_("non-member function %qD cannot have ref-qualifier")
>
> I have been able to figure out it relates to translation, but not
> exactly what the protocol around them is. I think in my original patch
> I had refactored this code a bunch, I figured adding a 3rd case to it
> justifies a refactor. I think I forgot to add those changes to the
> original patch, either that or I undid it or moved it somewhere else.
> Anyway, the point is, coming back to it now to re-add those diagnostics
> I realized I probably shouldn't have changed those strings.
>
> I also have been wondering whether I should be putting macros on any
> strings I add, it seemed like there might have been a macro for text
> that needs translation. Is this something I should be doing?
There are different kinds of format strings in GCC, the most common
are the gcc-internal-format strings. If you call a function which
is expected to take such translatable format string (in particular
a function which takes a gmsgid named argument like error, error_at,
pedwarn, warning_at, ...) and pass a string literal to that function,
nothing needs to be marked in a special way, both gcc/po/exgettext
is able to collect such literals into gcc/po/gcc.pot for translations
and the function is supposed to use gettext etc. to translate it
- e.g. see diagnostic_set_info using _(gmsgid) for that.
But, if there is e.g. a temporary pointer var which points to format
strings and only that is eventually passed to the diagnostic functions,
gcc/po/exgettext won't be able to collect such literals, which is where
the G_() macro comes into play and one marks the string as
gcc-internal-format with it; the translation is still handled by the
diagnostic function at runtime. The N_() macro is similar but for c-format
strings instead. The _() macro both collects for translations if it is
used with string literal, and expands to gettext call to translate it at
runtime, which is something that should be avoided if something translates
it again.
And another i18n rule is that one shouldn't try to construct diagnostic
messages from parts of english sentences, it is fine to fill in with %s/%qs
etc. language keywords etc. but otherwise the format string should contain
the whole diagnostic line, so that translators can reorder the words etc.
Jakub
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> > I will try to get something done today, but I was struggling with
> > writing some of the tests, there's also a lot more of them now. I also
> > wrote a bunch of musings in comments that I would like feedback on.
> >
> > My most concrete question is, how exactly should I be testing a
> > pedwarn, I want to test that I get the correct warning and error with
> > the separate flags, do I have to create two separate tests for each one?
>
>
> Yes. I tend to use letter suffixes for tests that vary only in flags
> (and expected results), e.g. feature1a.C, feature1b.C.
Will do.
> Instead of OPT_Wpedantic, this should be controlled by
> -Wc++23-extensions (OPT_Wc__23_extensions)
Yeah, I'll do this.
> If you wanted, you could add a more specific warning option for this
> (e.g. -Wc++23-explicit-this) which is also affected by
> -Wc++23-extensions, but I would lean toward just using the existing
> flag. Up to you.
I brought it up in irc and there was some pushback to my point of view
on it, so I'll just stick with OPT_Wc__23_extensions for now. I do
think a more sophisticated interface would be beneficial but I will
bring discussion around that up again in the future.
I've seen plenty of these G_ or _ macros on strings around like in
grokfndecl for these errors.
G_("static member function %qD cannot have cv-qualifier")
G_("non-member function %qD cannot have cv-qualifier")
G_("static member function %qD cannot have ref-qualifier")
G_("non-member function %qD cannot have ref-qualifier")
I have been able to figure out it relates to translation, but not
exactly what the protocol around them is. I think in my original patch
I had refactored this code a bunch, I figured adding a 3rd case to it
justifies a refactor. I think I forgot to add those changes to the
original patch, either that or I undid it or moved it somewhere else.
Anyway, the point is, coming back to it now to re-add those diagnostics
I realized I probably shouldn't have changed those strings.
I also have been wondering whether I should be putting macros on any
strings I add, it seemed like there might have been a macro for text
that needs translation. Is this something I should be doing?
Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 10/18/23 07:46, waffl3x wrote: Any progress on this, or do I need to coax the process along? :) Yeah, I've been working on it since the copyright assignment process has finished, originally I was going to note that on my next update which I had hoped to finish today or tomorrow. Well, in truth I was hoping to send one the same day that copyright assignment finished, but I found a nasty bug so I spent all day adding test cases for all the relevant overloadable operators. Currently, it crashes when calling a subscript operator declared with an explicit object parameter in a dependent context. I haven't looked into the fix yet, but I plan to. Also, before I forget, what is the process for confirming my copyright assignment on your end? Do you just need to check in with the FSF to see if it went through? Let me know if there's anything you need from me regarding that. Aside from the bug that's currently present in the first patch, I only have like 1 or 2 little things I want to change about it. I will make those few changes to patch 1, finish patch 2 (diagnostics) which will also include test cases for the new bug I found. After I am done that I plan on adding the things that are missing, because I suspect that looking into that will get me close to finding the fix for the crash. Hmm, is it? I see that clang thinks so, but I don't know where they get that idea from. The grammar certainly allows it: attribute-specifier-seqopt decl-specifier-seq declarator = initializer-clause and I don't see anything else that prohibits it. You would be right for P0847R7, but CWG DR 2653 changed that. You can find the updated grammar in dcl.fct section 3 (subsection? I'm not really sure to be honest.) I've also included a copy of the grammar here for your convenience. https://eel.is/c++draft/dcl.fct#nt:parameter-declaration parameter-declaration: attribute-specifier-seqopt thisopt decl-specifier-seq declarator attribute-specifier-seqopt decl-specifier-seq declarator = initializer-clause attribute-specifier-seqopt thisopt decl-specifier-seq abstract-declaratoropt attribute-specifier-seqopt decl-specifier-seq abstract-declaratoropt = initializer-clause Ah, yes, thanks. I was thinking to set a TREE_LANG_FLAG on the TREE_LIST node. I did figure this is originally what you meant, and I can still change it to go this route since I'm sure it's likely just as good. But I do recall something I didn't like in the implementation that nudged me towards using the purpose member instead. Either way, not a big deal. I think I just liked not having to mess with a linked list as I am not used to them as a data structure, it might have been that simple. :^) I wouldn't expect to need any actual dealing with the linked list, just setting a flag in cp_parameter_declaration_list at the same point as the existing PARENTHESIZED_LIST_P flag. But given CWG2653 as you pointed out, your current approach is fine. I will try to get something done today, but I was struggling with writing some of the tests, there's also a lot more of them now. I also wrote a bunch of musings in comments that I would like feedback on. My most concrete question is, how exactly should I be testing a pedwarn, I want to test that I get the correct warning and error with the separate flags, do I have to create two separate tests for each one? Yes. I tend to use letter suffixes for tests that vary only in flags (and expected results), e.g. feature1a.C, feature1b.C. I'm just going to include the little wall I wrote in decl.cc regarding pedwarn, just in case I can't get this done tonight so I can get some feedback regarding it. On the other hand, it might just not be very relevant to this patch in particular as I kind of noted, but maybe there's some way to do what I was musing about that I've overlooked. It does end up a bit ranty I guess, hopefully that doesn't make it confusing. ``` /* I believe we should make a switch for this feature specifically, I recall seeing discussion regarding enabling newer language features when set to older standards. I would advocate for a specific flag for each specific feature. Maybe they should all be under an override flag? -foverride-dialect=xobj,ifconstexpr (?) I dont think it makes sense to give each feature override it's own flag. I don't recall what the consensus was around this discussion either though. For the time being it's controlled by pedantic. I am concerned that tying this to pedantic going forward that one might want to enable -pedantic-errors while also enabling select features from newer dialects. It didn't look like this use case is supported to me. I suppose this will require redesign work to support, so for the purposes of this patch, emitting a pedwarn seems correct. I just don't like that it can't be suppressed on an individual basis. */ if (xobj_parm && cxx_dialect < cxx23) pedwarn(DECL_SOURCE_LOCATION
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> Any progress on this, or do I need to coax the process along? :) Yeah, I've been working on it since the copyright assignment process has finished, originally I was going to note that on my next update which I had hoped to finish today or tomorrow. Well, in truth I was hoping to send one the same day that copyright assignment finished, but I found a nasty bug so I spent all day adding test cases for all the relevant overloadable operators. Currently, it crashes when calling a subscript operator declared with an explicit object parameter in a dependent context. I haven't looked into the fix yet, but I plan to. Also, before I forget, what is the process for confirming my copyright assignment on your end? Do you just need to check in with the FSF to see if it went through? Let me know if there's anything you need from me regarding that. Aside from the bug that's currently present in the first patch, I only have like 1 or 2 little things I want to change about it. I will make those few changes to patch 1, finish patch 2 (diagnostics) which will also include test cases for the new bug I found. After I am done that I plan on adding the things that are missing, because I suspect that looking into that will get me close to finding the fix for the crash. > Hmm, is it? I see that clang thinks so, but I don't know where they get > that idea from. The grammar certainly allows it: > > > attribute-specifier-seqopt decl-specifier-seq declarator = > > initializer-clause > > > and I don't see anything else that prohibits it. You would be right for P0847R7, but CWG DR 2653 changed that. You can find the updated grammar in dcl.fct section 3 (subsection? I'm not really sure to be honest.) I've also included a copy of the grammar here for your convenience. https://eel.is/c++draft/dcl.fct#nt:parameter-declaration parameter-declaration: attribute-specifier-seqopt thisopt decl-specifier-seq declarator attribute-specifier-seqopt decl-specifier-seq declarator = initializer-clause attribute-specifier-seqopt thisopt decl-specifier-seq abstract-declaratoropt attribute-specifier-seqopt decl-specifier-seq abstract-declaratoropt = initializer-clause > I was thinking to set a TREE_LANG_FLAG on the TREE_LIST node. I did figure this is originally what you meant, and I can still change it to go this route since I'm sure it's likely just as good. But I do recall something I didn't like in the implementation that nudged me towards using the purpose member instead. Either way, not a big deal. I think I just liked not having to mess with a linked list as I am not used to them as a data structure, it might have been that simple. :^) I will try to get something done today, but I was struggling with writing some of the tests, there's also a lot more of them now. I also wrote a bunch of musings in comments that I would like feedback on. My most concrete question is, how exactly should I be testing a pedwarn, I want to test that I get the correct warning and error with the separate flags, do I have to create two separate tests for each one? I'm just going to include the little wall I wrote in decl.cc regarding pedwarn, just in case I can't get this done tonight so I can get some feedback regarding it. On the other hand, it might just not be very relevant to this patch in particular as I kind of noted, but maybe there's some way to do what I was musing about that I've overlooked. It does end up a bit ranty I guess, hopefully that doesn't make it confusing. ``` /* I believe we should make a switch for this feature specifically, I recall seeing discussion regarding enabling newer language features when set to older standards. I would advocate for a specific flag for each specific feature. Maybe they should all be under an override flag? -foverride-dialect=xobj,ifconstexpr (?) I dont think it makes sense to give each feature override it's own flag. I don't recall what the consensus was around this discussion either though. For the time being it's controlled by pedantic. I am concerned that tying this to pedantic going forward that one might want to enable -pedantic-errors while also enabling select features from newer dialects. It didn't look like this use case is supported to me. I suppose this will require redesign work to support, so for the purposes of this patch, emitting a pedwarn seems correct. I just don't like that it can't be suppressed on an individual basis. */ if (xobj_parm && cxx_dialect < cxx23) pedwarn(DECL_SOURCE_LOCATION (xobj_parm), OPT_Wpedantic, ""); ``` That's all for now, I will try, (but I am very much not promising,) to have an update by the end of today (6-8 hours for me.) If I manage to get that out, I will (finally) start moving forward on implementing the missing and broken features, and the aforementioned bug. Alex
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 9/25/23 21:56, waffl3x wrote: On the plus side, I took my time to figure out how to best to pass down information about whether a param is an xobj param. My initial impression on what you were suggesting was to push another node on the front of the list, but I stared at it for a few hours and didn't think it would work out. I was thinking to set a TREE_LANG_FLAG on the TREE_LIST node. However, eventually I realized that the purpose member if free for xobj params as it is illegal for them to have default arguments. Hmm, is it? I see that clang thinks so, but I don't know where they get that idea from. The grammar certainly allows it: attribute-specifier-seqopt decl-specifier-seq declarator = initializer-clause and I don't see anything else that prohibits it. Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
On 9/25/23 21:56, waffl3x wrote: Also, just a quick update on my copyright assignment, I have sent an e-mail to the FSF and haven't gotten a response yet. From what I was reading, I am confident that it's my preferred option going forward though. Hopefully they get back to me soon. Any progress on this, or do I need to coax the process along? :) Jason
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
Not to worry, I'm currently going through that process with the FSF, it was confirmed that a pseudonym should be just fine. I don't know how long the process takes but my goal is to get this in for GCC14, and surely this won't take more than a month. One can only hope anyway. On 2023-09-27 04:43 p.m., Hans-Peter Nilsson wrote: >> Date: Tue, 26 Sep 2023 01:56:55 + >> From: waffl3x > >> Signed-off-by: waffl3x > > I think I've read that you have to put your actual name in > the DCO; using an alias (presumably) as above would be > wrong. > > Ah, it's on https://gcc.gnu.org/dco.html - the *second* DCO > link; under "Signed-off-by", on > https://gcc.gnu.org/contribute.html! "sorry, no pseudonyms > or anonymous contributions". > > (Also, from Some Source I Don't Remember: using an alias if > you have FSF papers in place is ok; you can use a pseudonym > if FSF can match it to papers on file that have your actual > name or something to that effect.) > > brgds, H-P
Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> Date: Tue, 26 Sep 2023 01:56:55 + > From: waffl3x > Signed-off-by: waffl3x I think I've read that you have to put your actual name in the DCO; using an alias (presumably) as above would be wrong. Ah, it's on https://gcc.gnu.org/dco.html - the *second* DCO link; under "Signed-off-by", on https://gcc.gnu.org/contribute.html! "sorry, no pseudonyms or anonymous contributions". (Also, from Some Source I Don't Remember: using an alias if you have FSF papers in place is ok; you can use a pseudonym if FSF can match it to papers on file that have your actual name or something to that effect.) brgds, H-P
[PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]
> Yes, but I'll warn you that grokdeclarator has resisted refactoring for
> a long time...
That will certainly be what I work on after this is squared off then,
I've been up and down grokdeclarator so I'm confident I'll be able to
do it.
As for the patch, I sure took my sweet time with it, but here it is. I
hope to work on the diagnostics patch tomorrow, but as you've probably
figured out it's best not to take my word on timeframes :^).
On the plus side, I took my time to figure out how to best to pass down
information about whether a param is an xobj param. My initial
impression on what you were suggesting was to push another node on the
front of the list, but I stared at it for a few hours and didn't think
it would work out. However, eventually I realized that the purpose
member if free for xobj params as it is illegal for them to have
default arguments. So I ended up passing it over the TREE_LIST after
all, maybe this is what you meant in the first place anyway too.
I am pretty confident that this version is all good, with only a few
possible issues.
An update on my copyright assignment, I sent an e-mail and haven't
gotten a response yet. From what I saw, I am confident that it's my
preferred option going forward though. Hopefully they get back to me
soon.
Also, just a quick update on my copyright assignment, I have sent an
e-mail to the FSF and haven't gotten a response yet. From what I was
reading, I am confident that it's my preferred option going forward
though. Hopefully they get back to me soon.
Bootstrapped and regtested on x86_64-pc-linux-gnu.
From bbfbcc72e8c0868559284352c71731394c98441e Mon Sep 17 00:00:00 2001
From: waffl3x
Date: Mon, 25 Sep 2023 16:59:10 -0600
Subject: [PATCH] c++: Initial support for C++23 P0847R7 (Deducing This)
[PR102609]
This patch implements initial support for P0847R7, without additions to
diagnostics. Almost everything should work correctly, barring a few
limitations which are listed below. I attempted to minimize changes to the
existing code, treating explicit object member functions as static functions,
while flagging them to give them extra powers seemed to be the best way of
achieving this. For this patch, the flag is only utilized in call.cc for
resolving overloads and making the actual function call.
Internally, the difference between a static member function and an implicit
object member function appears to be whether the type node of the decl is a
FUNCTION_TYPE or a METHOD_TYPE. So to get the desired behavior, it seems to be
sufficient to simply prevent conversion from FUNC_TYPE to METHOD_TYPE in
grokdeclarator when the first parameter is an explicit object parameter. To
achieve this, explicit object parameters are flagged as such through each the
TREE_LIST's purpose member in declarator->u.function.parameters. Typically the
purpose member is used for default arguments, as those are not allowed for
explicit object parameters, we are able to repurpose purpose for our purposes.
The value used as a flag is the "this_identifier" global tree, as it seemed to
be the most fitting of the current global trees. Even though it is obviously
illegal for any parameter except the first to be an explicit object parameter,
each parameter parsed as an explicit object parameter will be flagged in this
manner. This will be used for diagnostics in the following patch. When an
explicit object parameter is encountered in grokdeclarator, the purpose member
is nulled before the list is passed elsewhere to maintain compatibility with
any code that assumes that a non-null purpose member indicates a default
argument. This patch only checks for and nulls the first parameter however.
As for the previously mentioned limitations, lambdas do not work correctly yet,
but I suspect that a few tweaks are all it will take to have them fully
functional. User defined conversion functions are not called when an explicit
object member function with an explicit object parameter of an unrelated type
is called. The following case does not behave correctly because of this.
struct S {
operator size_t() {
return 42;
}
size_t f(this size_t n) {
return n;
}
};
int main()
{
S s{};
size_t a = s.f();
}
Currently, it appears that the object argument is simply reinterpreted as
a size_t instead of properly calling the user defined conversion function.
The validity of such a conversion is still considered however, if there is no
way to convert S to a size_t an appropriate compile error will be emitted.
I have an idea of what changes need to be made to fix this, but I did not
persue this for the initial implementation patch.
This bug can be observed in the explicit-object-param4.C test case, while
explicit-object-param3.C demonstrates the non functioning lambdas.
PR c++/102609
gcc/cp/ChangeLog:
PR c++/102609
Initial support for C++23 P0847R7 - Deducing this.
* call.cc (add_candidates): Check if fn is an xobj member function.
(build_over_call): Ditto.
*
