[v5,1/1] c++: Initial support for P0847R7 (Deducing This) [PR102609]

Bootstrapped and tested on x86_64-linux with no regressions.

Hopefully this patch is legible enough for reviewing purposes, I've not
been feeling the greatest so it was a task to get this finished.
Tomorrow I will look at putting the diagnostics in
start_preparsed_function and also fixing up anything else.

To reiterate in case it wasn't abundantly clear by the barren changelog
and commit message, this version is not intended as the final revision.

Handling re-declarations was kind of nightmarish, so the comments in
there are lengthy, but I am fairly certain I implemented them correctly.

I am going to get some sleep now, hopefully I will feel better tomorrow
and be ready to polish off the patch. Thanks for the patience.

Alex

On 11/21/23 08:04, waffl3x wrote:
> Bootstrapped and tested on x86_64-linux with no regressions.
> 
> Hopefully this patch is legible enough for reviewing purposes, I've not
> been feeling the greatest so it was a task to get this finished.
> Tomorrow I will look at putting the diagnostics in
> start_preparsed_function and also fixing up anything else.
> 
> To reiterate in case it wasn't abundantly clear by the barren changelog
> and commit message, this version is not intended as the final revision.
> 
> Handling re-declarations was kind of nightmarish, so the comments in
> there are lengthy, but I am fairly certain I implemented them correctly.
> 
> I am going to get some sleep now, hopefully I will feel better tomorrow
> and be ready to polish off the patch. Thanks for the patience.

Great!

> 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.

I see what you mean, instantiate body isn't prepared for 
start_preparsed_function to fail.  It's ok to handle this in two places.
Though actually, instantiate_body is too late for it to fail; I think 
for the template case it should fail in tsubst_lambda_expr, before we 
even start to consider the body.

Incidentally, I notice this code in tsubst_function_decl seems to need 
adjusting for xobj:

   tree parms = DECL_ARGUMENTS (t);
   if (closure && !DECL_STATIC_FUNCTION_P (t))
     parms = DECL_CHAIN (parms);
   parms = tsubst (parms, args, complain, t);
   for (tree parm = parms; parm; parm = DECL_CHAIN (parm))
     DECL_CONTEXT (parm) = r;
   if (closure && !DECL_STATIC_FUNCTION_P (t))
...

and this in tsubst_lambda_expr that assumes iobj:

       /* Fix the type of 'this'.  */
       fntype = build_memfn_type (fntype, type,
                                  type_memfn_quals (fntype),
                                  type_memfn_rqual (fntype));

This also seems like the place to check for unrelated type.

>  /* Nonzero for FUNCTION_DECL means that this decl is a non-static
> -   member function.  */
> +   member function, use DECL_IOBJ_MEMBER_FUNC_P instead.  */
>  #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
>    (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
>  
> +/* Nonzero for FUNCTION_DECL means that this decl is an implicit object
> +   member function.  */
> +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> +  (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)

I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than 
add a new, equivalent one.  And then go through all the current uses of 
the old macro to decide whether they mean IOBJ or OBJECT.

> -   (static or non-static).  */
> +   (static or object).  */

Let's leave this comment as it was.

> +  auto handle_arg = [fn, flags, complain](tree type,
> +                                         tree arg,
> +                                         int const param_index,
> +                                         conversion *conv,
> +                                         bool const conversion_warning)

Let's move the conversion_warning logic into the handle_arg lambda 
rather than have it as a parameter.  Yes, we don't need it for the xobj 
parm, but I think it's cleaner to have less in the loop.

Also, let's move handle_arg after the iobj 'this' handling so it's 
closer to the uses.  For which the 'else xobj' needs to drop the 'else', 
or change to 'if (first_arg)'.

> +      /* We currently handle for the case where first_arg is NULL_TREE
> +	 in the future this should be changed and the assert reactivated.  */
> +      #if 0
> +      gcc_assert (first_arg);
> +      #endif

Let's leave this out.

> +      val = handle_arg(TREE_VALUE (parm),

Missing space before (.

> -      if (null_node_p (arg)
> -          && DECL_TEMPLATE_INFO (fn)
> -          && cand->template_decl
> -	  && !cand->explicit_targs)
> -        conversion_warning = false;
> -
> +      auto determine_conversion_warning = [&]()
> +	{
> +	  return !(null_node_p (current_arg)
> +	    && DECL_TEMPLATE_INFO (fn)
> +	    && cand->template_decl
> +	    && !cand->explicit_targs);
> +	};

I don't think this needs to be a lambda.

> +	     declaration, then the type of it's object parameter is still
...
> +		 but due to [basic.scope.scope.3] we need to ignore it's
...
> +	    /* Since a valid xobj parm has it's purpose cleared in find_xobj_parm

"its"

> -  if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
> -      && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
> +  if (DECL_OBJECT_MEMBER_FUNC_P (fn)
> +      && !(complain & tf_ptrmem_ok))

The following code (not quoted) could share more code between the cases 
if we check

!(flag_ms_extensions && DECL_IOBJ_MEMBER_FUNC_P (fn))

here?

> +		else if (declarator->declarator->kind == cdk_pointer)
> +		  error_at (DECL_SOURCE_LOCATION (xobj_parm),
> +			 /* "a pointer to function type cannot "?  */
> +			    "a function pointer type cannot "
> +			    "have an explicit object parameter");

"pointer to function type", yes.

> +		/* The locations being used here are probably not correct.  */

Why not?

Let's clear xobj_parm after giving an error in the TYPENAME case

>   if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
>       || DECL_STATIC_FUNCTION_P (decl))

I think this can just be if (DECL_OBJECT_MEMBER_FUNC_P (decl)).

>    if (TREE_CODE (fntype) == METHOD_TYPE)
>      ctype = TYPE_METHOD_BASETYPE (fntype);
> +  else if (DECL_XOBJ_MEMBER_FUNC_P (decl1))
> +    ctype = DECL_CONTEXT (decl1);

All of this can be

if (DECL_CLASS_SCOPE_P (decl1))
   ctype = DECL_CONTEXT (decl1);

I think I'm going to go ahead and clean that up now.

> +  /* We don't need deal with 'this' or vtable for xobj member functions.  */
> +  if (ctype && !doing_friend &&
> +      !(DECL_STATIC_FUNCTION_P (decl1) || DECL_XOBJ_MEMBER_FUNC_P (decl1)))

And this can be

if (DECL_IOBJ_MEMBER_FUNC_P (decl1))

> +  if (INDIRECT_TYPE_P (TREE_TYPE (object)))
> +    /* The proxy variable forwards to the capture field.  */

This comment should stay outside the if.

> +      /* This might not be the most appropriate place for this, but I'm going
> +	 to hold back on changing it for the time being since we are short on
> +	 time. It's not really a parser error, it's more a semantic error.
> +	 But this is where the other errors were so...  */

It's OK to give semantic errors in the parser if they are always 
detectable at parse time (before template instantiation).

Though maybe we could contain the xobj vs mutable and xobj vs static 
diagnostics in this block:

> +  if (xobj_param)
> +    {
> +      quals = TYPE_UNQUALIFIED;
> +      if (TYPE_REF_P (xobj_param)
> +	  && !(cp_type_quals (TREE_TYPE (xobj_param)) & TYPE_QUAL_CONST))
> +        LAMBDA_EXPR_MUTABLE_P (lambda_expr) = 1;
> +    }

rather than separately in the mutable/static blocks?  But the way you 
have it is fine, too.

> +	  inform (DECL_SOURCE_LOCATION(xobj_param),

missing space before (xobj_param), repeated several times in the rest of 
the function.

> +	  /* Error reporting here is a little awkward, if the type of the
> +	     object parameter is deduced, we should tell them the lambda
> +	     is effectively already const, or to make the param const if it is
> +	     not, but if it is deduced and taken by value shouldn't we say
> +	     that it's taken by copy and won't mutate?
> +	     Seems right to me, but it's a little strange.  */

I think just omit the inform if dependent_type_p.

> -	if (!LAMBDA_EXPR_STATIC_P (lambda_expr))
> +	if (!LAMBDA_EXPR_STATIC_P (lambda_expr)
> +	    && !DECL_XOBJ_MEMBER_FUNC_P (fco))

if (DECL_IOBJ_MEMBER_FUNC_P (fco))

> +      /* Since a lambda's type is anonymous, we can assume an explicit object
> +	 parameter is unrelated... at least I believe so. I can think of a
> +	 paradoxical case that almost works, where a class is forward declared
> +	 and then defined deriving from a lambda that has an explicit object
> +	 parameter of that class type, but the only way that can work is with
> +	 decltype... okay yeah I think it can work for a struct defined in
> +	 block scope, but I'm leaving this as is until I can confirm my
> +	 hypothesis.  */

Like this?

template <class T> struct A: T { };
template <class T> T f()
{
   int x = 42;
   auto f1 = [x]<typename U>(this A<U>&& self) { return x; };
   return A<decltype(f1)>{f1}();
}
int main() { return f<int>(); }

When I disable the error this crashes in register_parameter_specializations

> +      if (!TEMPLATE_PARM_P (non_reference (TREE_TYPE (xobj_param))))

...so let's just check dependent_type_p here.

> +      error ("%<this%> is unavailable for explicit object member "
> +	     "functions");
> +      /* I can imagine doing a fixit here, suggesting replacing
> +	 this / *this / this-> with &name / name / "name." but it would be
> +	 very difficult to get it perfect and I've been advised against
> +	 making imperfect fixits.
> +	 Perhaps it would be as simple as the replacements listed,
> +	 even if one is move'ing/forward'ing, if the replacement is just
> +	 done in the same place, it will be exactly what the user wants?
> +	 Even if this is true though, there's still a problem of getting the
> +	 context of the expression to find which tokens to replace.
> +	 I would really like for this to be possible though.
> +	 I will decide whether or not to persue this after review.  */

I still think (&self) will always be a correct replacement for 'this', 
but unlikely to be the way they actually want their code to look.  To 
detect this-> or *this should be doable in the parser (where you can 
look forward and back in the token stream) but we don't have access to 
that here in finish_this_expr.

> +	/* Pull out the function_decl for a single xobj member function,
> +	   similar to the BASELINK case above. If we did the same optimization
> +	   as we do for single static member functions (passing in a BASELINK)
> +	   then it could be handled the same too.  */

This comment seems out of date given that the BASELINK handling just 
gives an error.  We can't use the same handling as static member 
functions because we need the SCOPE_REF to tell us that it was properly 
qualified.

Jason

On Tuesday, November 21st, 2023 at 8:22 PM, Jason Merrill <jason@redhat.com> wrote:


>
>
> On 11/21/23 08:04, waffl3x wrote:
>
> > Bootstrapped and tested on x86_64-linux with no regressions.
> >
> > Hopefully this patch is legible enough for reviewing purposes, I've not
> > been feeling the greatest so it was a task to get this finished.
> > Tomorrow I will look at putting the diagnostics in
> > start_preparsed_function and also fixing up anything else.
> >
> > To reiterate in case it wasn't abundantly clear by the barren changelog
> > and commit message, this version is not intended as the final revision.
> >
> > Handling re-declarations was kind of nightmarish, so the comments in
> > there are lengthy, but I am fairly certain I implemented them correctly.
> >
> > I am going to get some sleep now, hopefully I will feel better tomorrow
> > and be ready to polish off the patch. Thanks for the patience.
>
>
> Great!
>
> > 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.
>
>
> I see what you mean, instantiate body isn't prepared for
> start_preparsed_function to fail. It's ok to handle this in two places.
> Though actually, instantiate_body is too late for it to fail; I think
> for the template case it should fail in tsubst_lambda_expr, before we
> even start to consider the body.
>
> Incidentally, I notice this code in tsubst_function_decl seems to need
> adjusting for xobj:
>
> tree parms = DECL_ARGUMENTS (t);
> if (closure && !DECL_STATIC_FUNCTION_P (t))
> parms = DECL_CHAIN (parms);
> parms = tsubst (parms, args, complain, t);
> for (tree parm = parms; parm; parm = DECL_CHAIN (parm))
> DECL_CONTEXT (parm) = r;
> if (closure && !DECL_STATIC_FUNCTION_P (t))
> ...
>
> and this in tsubst_lambda_expr that assumes iobj:
>
> /* Fix the type of 'this'. */
> fntype = build_memfn_type (fntype, type,
> type_memfn_quals (fntype),
> type_memfn_rqual (fntype));

Originally I was going to say this doesn't look like a problem in
tsubst_lambda_expr, but after looking at tsubst_function_decl I'm
thinking it might be the source of some trouble. If it really was
causing problems I would think it would be working much worse than it
currently is, but it does feel like it might be the actual source of
the bug I was chasing yesterday. Assigning to a capture with a deduced
const xobj parameter is not being rejected right now, this might be
causing it. I'll look more thoroughly today.

> This also seems like the place to check for unrelated type.

It does feel that way, I agree.

> > /* Nonzero for FUNCTION_DECL means that this decl is a non-static
> > - member function. */
> > + member function, use DECL_IOBJ_MEMBER_FUNC_P instead. */
> > #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
> > (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> >
> > +/* Nonzero for FUNCTION_DECL means that this decl is an implicit object
> > + member function. */
> > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> > + (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
>
>
> I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than
> add a new, equivalent one. And then go through all the current uses of
> the old macro to decide whether they mean IOBJ or OBJECT.

I figure it would be easier to make that transition if there's a clear
line between old versus new. To be clear, my intention is for the old
macro to be removed once all the uses of it are changed over to the new
macro. I can still remove it for the patch if you like but having both
and removing the old one later seems better to me.

> > - (static or non-static). */
> > + (static or object). */
>
>
> Let's leave this comment as it was.

Okay.

> > + auto handle_arg = [fn, flags, complain](tree type,
> > + tree arg,
> > + int const param_index,
> > + conversion *conv,
> > + bool const conversion_warning)
>
>
> Let's move the conversion_warning logic into the handle_arg lambda
> rather than have it as a parameter. Yes, we don't need it for the xobj
> parm, but I think it's cleaner to have less in the loop.

I would argue that it's cleaner to have the lambda be concise, but I'll
make this change.

> Also, let's move handle_arg after the iobj 'this' handling so it's
> closer to the uses. For which the 'else xobj' needs to drop the 'else',
> or change to 'if (first_arg)'.

Agreed, I didn't like how far away it was.

> > + /* We currently handle for the case where first_arg is NULL_TREE
> > + in the future this should be changed and the assert reactivated. */
> > + #if 0
> > + gcc_assert (first_arg);
> > + #endif
>
>
> Let's leave this out.

Alright.

> > + val = handle_arg(TREE_VALUE (parm),
>
>
> Missing space before (.
>
> > - if (null_node_p (arg)
> > - && DECL_TEMPLATE_INFO (fn)
> > - && cand->template_decl
> > - && !cand->explicit_targs)
> > - conversion_warning = false;
> > -
> > + auto determine_conversion_warning = &
> > + {
> > + return !(null_node_p (current_arg)
> > + && DECL_TEMPLATE_INFO (fn)
> > + && cand->template_decl
> > + && !cand->explicit_targs);
> > + };
>
>
> I don't think this needs to be a lambda.

Yeah probably a little unnecessary, I'll switch it back.

> > + declaration, then the type of it's object parameter is still
>
> ...
>
> > + but due to [basic.scope.scope.3] we need to ignore it's
>
> ...
>
> > + /* Since a valid xobj parm has it's purpose cleared in find_xobj_parm
>
>
> "its"

TIL while we use an apostrophe for possessive nouns, we don't use one
for the posessive its. English!

> > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
> > - && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
> > + if (DECL_OBJECT_MEMBER_FUNC_P (fn)
> > + && !(complain & tf_ptrmem_ok))
>
>
> The following code (not quoted) could share more code between the cases
> if we check
>
> !(flag_ms_extensions && DECL_IOBJ_MEMBER_FUNC_P (fn))
>
> here?

Yeah I didn't like it very much when I wrote it, but I was too tired to
reliably merge things together so I just wanted it to work. I will
screw around with it more today.

> > + else if (declarator->declarator->kind == cdk_pointer)
> > + error_at (DECL_SOURCE_LOCATION (xobj_parm),
> > + /* "a pointer to function type cannot "? */
> > + "a function pointer type cannot "
> > + "have an explicit object parameter");
>
>
> "pointer to function type", yes.
>
> > + /* The locations being used here are probably not correct. */
>
>
> Why not?

I threw them in just so I could call inform, but it doesn't feel like
the messages should be pointing at the parameter, but rather at the
full type declaration. When I put those locations in I wasn't sure how
to get the full declaration location, and I'm still not 100% confident
in how to do it, so I just threw them in and moved on.

> Let's clear xobj_parm after giving an error in the TYPENAME case

I don't like the spirit of this very much, whats your reasoning for
this? We're nearly at the end of the scope where it is last used, I
think it would be more unclear if we suddenly set it to NULL_TREE near
the end. It raises the question of whether that assignment actually
does anything, or if we are just trying to indicate that it isn't being
used anymore, but I already made sure to declare it in the deepest
scope possible. That much should be sufficient for indicating it's
usage, no?

> > if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
> > || DECL_STATIC_FUNCTION_P (decl))
>
>
> I think this can just be if (DECL_OBJECT_MEMBER_FUNC_P (decl)).

Alright, and going forward I'll try to make more changes that are
consistent with this one. With that said I'm not sure it can, but I'll
take a close look and if you're right I'll make that change.

> > if (TREE_CODE (fntype) == METHOD_TYPE)
> > ctype = TYPE_METHOD_BASETYPE (fntype);
> > + else if (DECL_XOBJ_MEMBER_FUNC_P (decl1))
> > + ctype = DECL_CONTEXT (decl1);
>
>
> All of this can be
>
> if (DECL_CLASS_SCOPE_P (decl1))
> ctype = DECL_CONTEXT (decl1);
>
> I think I'm going to go ahead and clean that up now.

Sounds good to me, a lot of this stuff needs small cleanups and I'm
just concerned about making them too much.

> > + /* We don't need deal with 'this' or vtable for xobj member functions. */
> > + if (ctype && !doing_friend &&
> > + !(DECL_STATIC_FUNCTION_P (decl1) || DECL_XOBJ_MEMBER_FUNC_P (decl1)))
>
>
> And this can be
>
> if (DECL_IOBJ_MEMBER_FUNC_P (decl1))
>
> > + if (INDIRECT_TYPE_P (TREE_TYPE (object)))
> > + /* The proxy variable forwards to the capture field. */
>
>
> This comment should stay outside the if.
>
> > + /* This might not be the most appropriate place for this, but I'm going
> > + to hold back on changing it for the time being since we are short on
> > + time. It's not really a parser error, it's more a semantic error.
> > + But this is where the other errors were so... */
>
>
> It's OK to give semantic errors in the parser if they are always
> detectable at parse time (before template instantiation).

Okay, it's good to hear a definite answer to that, I'll keep it in mind.

> Though maybe we could contain the xobj vs mutable and xobj vs static
> diagnostics in this block:
>
> > + if (xobj_param)
> > + {
> > + quals = TYPE_UNQUALIFIED;
> > + if (TYPE_REF_P (xobj_param)
> > + && !(cp_type_quals (TREE_TYPE (xobj_param)) & TYPE_QUAL_CONST))
> > + LAMBDA_EXPR_MUTABLE_P (lambda_expr) = 1;
> > + }
>
>
> rather than separately in the mutable/static blocks? But the way you
> have it is fine, too.

Yeah I don't like the code very much, I am happy to refactor it, it's
just another case of tired coding so I didn't move things around too
much.

> > + inform (DECL_SOURCE_LOCATION(xobj_param),
>
>
> missing space before (xobj_param), repeated several times in the rest of
> the function.

Yeah tired coding. :)
I'll be sure to go over everything with a fine tooth comb again before
the final version of the patch.

> > + /* Error reporting here is a little awkward, if the type of the
> > + object parameter is deduced, we should tell them the lambda
> > + is effectively already const, or to make the param const if it is
> > + not, but if it is deduced and taken by value shouldn't we say
> > + that it's taken by copy and won't mutate?
> > + Seems right to me, but it's a little strange. */
>
>
> I think just omit the inform if dependent_type_p.

Maybe I don't understand what a dependent type is as well as I thought,
but doesn't this defeat every useful case? The most common being an
xobj parameter of lambda type, which will always be deduced. Unless a
template parameter does not count as a dependent type, which is not
something I've ever thought about before.

It does need to be better though, so I'll work on it and I'll keep what
you said in mind.

> > - if (!LAMBDA_EXPR_STATIC_P (lambda_expr))
> > + if (!LAMBDA_EXPR_STATIC_P (lambda_expr)
> > + && !DECL_XOBJ_MEMBER_FUNC_P (fco))
>
>
> if (DECL_IOBJ_MEMBER_FUNC_P (fco))
>
> > + /* Since a lambda's type is anonymous, we can assume an explicit object
> > + parameter is unrelated... at least I believe so. I can think of a
> > + paradoxical case that almost works, where a class is forward declared
> > + and then defined deriving from a lambda that has an explicit object
> > + parameter of that class type, but the only way that can work is with
> > + decltype... okay yeah I think it can work for a struct defined in
> > + block scope, but I'm leaving this as is until I can confirm my
> > + hypothesis. */
>
>
> Like this?
>
> template <class T> struct A: T { };
>
> template <class T> T f()
>
> {
> int x = 42;
> auto f1 = [x]<typename U>(this A<U>&& self) { return x; };
>
> return A<decltype(f1)>{f1}();
>
> }
> int main() { return f<int>(); }

Oh boy, I had a feeling I was wrong. I was really hoping I was right
but here it is, that definitely counts. I had tried something like this
but I decided that it wasn't possible, I completely forgot about
template argument deduction and passing it in like this.

>
> When I disable the error this crashes in register_parameter_specializations
>
> > + if (!TEMPLATE_PARM_P (non_reference (TREE_TYPE (xobj_param))))
>
>
> ...so let's just check dependent_type_p here.

I think this jogs my memory on what a dependent type is. I believe I
see how this would work now.

Mildly related, a lot of the stuff I hacked together with multiple
levels of accessing macros and predicates was due to not being able to
find a solution for what I needed. I think we would highly benefit from
better documentation of the accessors and predicates. I believe I've
seen some that appear to be duplicates, and some where they don't
appear to be implemented properly or match their description. If there
is such a document please direct me to it as I have spent an hour or so
each time I stumble on one of these problems.

In the section I wrote in add_method I spent I think about 3 hours
trying to figure out what combination of predicates and accessing
macros was correct for what I was trying to do. It gets pretty
convoluted to traverse, especially when the implementations of them are
rather involved. Finding non_reference was a big help for example, and
I stumbled across it by pure luck.

If we don't have this kind of documentation anywhere yet, I'm happy to
work on it next. I've spent a lot of time looking at it all so I feel
like I have a DECENT grasp on some of it. I think it would greatly
enhance the status quo, and might even help us figure out what needs to
be cleaned up, removed, replaced, etc.

Side side note, if said document does exist already, I either need to
learn how to search the wiki more effectively or it needs some sort of
improvement.

> > + error ("%<this%> is unavailable for explicit object member "
> > + "functions");
> > + /* I can imagine doing a fixit here, suggesting replacing
> > + this / *this / this-> with &name / name / "name." but it would be
> > + very difficult to get it perfect and I've been advised against
> > + making imperfect fixits.
> > + Perhaps it would be as simple as the replacements listed,
> > + even if one is move'ing/forward'ing, if the replacement is just
> > + done in the same place, it will be exactly what the user wants?
> > + Even if this is true though, there's still a problem of getting the
> > + context of the expression to find which tokens to replace.
> > + I would really like for this to be possible though.
> > + I will decide whether or not to persue this after review. */
>
>
> I still think (&self) will always be a correct replacement for 'this',
> but unlikely to be the way they actually want their code to look. To
> detect this-> or *this should be doable in the parser (where you can
> look forward and back in the token stream) but we don't have access to
> that here in finish_this_expr.

Yeah, that is the issue. At the moment I'm putting any fixit stuff that
I want to do on the backburner. I've come across a few more I wanted to
do but, I've just come to the conclusion that learning how to do fixits
properly will be something I'll need to sit down and study on its own.
They are really finicky, the last one I tried to do just doesn't seem
to show up at all and I don't really know why.

Regarding (&self), yeah it will always be valid, it's just like you
said it's unlikely to be what the user means. I don't want to put a
half solution in here, I'll just delay the full solution.

> > + /* Pull out the function_decl for a single xobj member function,
> > + similar to the BASELINK case above. If we did the same optimization
> > + as we do for single static member functions (passing in a BASELINK)
> > + then it could be handled the same too. */
>
>
> This comment seems out of date given that the BASELINK handling just
> gives an error. We can't use the same handling as static member
> functions because we need the SCOPE_REF to tell us that it was properly
> qualified.
>
> Jason

I can revise how the comment reads, especially since as you've pointed
out introducing the baselink optimization to non-overloaded xobj member
functions would cause more problems than benefits. What I meant by the
comment was that it just simply pulls the function_decl out and puts it
in arg, then lets everything else after the switch case handle it. It
definitely isn't clear the way it's written right now, I will revise it.

I mostly got tests written yesterday, not a whole lot else done. I
found that one of my comments was false specifically regarding the
change in build_capture_proxy.

+  /* Making this conditional prevents the ICE, but does not actually work
+     correctly.  */

So funny enough, the comment seems to be false for the version of the
patch I submitted. I removed the change I made in
finish_non_static_data_member before submitting the patch, but didn't
realize that the change I made in there was what actually caused the
problem I was referencing. It wasn't until yesterday morning that I
realized that the behavior was actually correct. I also blame GDB for
acting funky, for some reason even in a build with optimizations off,
assignments to variables were appearing late. I blame GDB but it really
might not be its fault. Regardless, it threw me off quite a bit a few
times I think, I only noticed it when I started comparing return values
from functions (with the finish command) and what they were being
assigned to. It seems to happen when assigning to a variable that was
declared but not initialized.

Rambling aside, the point is, that simple change you suggested in
build_capture_proxy does appear to have been sufficient for fixing the
by-value xobj parameter case. The next patch will have tests to
demonstrate these cases. Realistically I should have had the tests done
for this version but, I just really couldn't manage it. With how much
feedback you gave I think I made the right call getting it sent faster
rather than waiting for all these things.

I will attempt to get another version finished today. I have a feeling
you were right about tsubst_lambda_expr and tsubst_function_decl so
hopefully that will expedite things. As noted, the next version will
include more tests so there will be less mystery in what works and what
doesn't.

Alex

On 11/22/23 15:46, waffl3x wrote:
> On Tuesday, November 21st, 2023 at 8:22 PM, Jason Merrill <jason@redhat.com> wrote:
>> On 11/21/23 08:04, waffl3x wrote:
>>
>>> /* Nonzero for FUNCTION_DECL means that this decl is a non-static
>>> - member function. */
>>> + member function, use DECL_IOBJ_MEMBER_FUNC_P instead. */
>>> #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
>>> (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
>>>
>>> +/* Nonzero for FUNCTION_DECL means that this decl is an implicit object
>>> + member function. */
>>> +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
>>> + (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
>>
>> I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than
>> add a new, equivalent one. And then go through all the current uses of
>> the old macro to decide whether they mean IOBJ or OBJECT.
> 
> I figure it would be easier to make that transition if there's a clear
> line between old versus new. To be clear, my intention is for the old
> macro to be removed once all the uses of it are changed over to the new
> macro. I can still remove it for the patch if you like but having both
> and removing the old one later seems better to me.

Hmm, I think changing all the uses is a necessary part of this change. 
I suppose it could happen before the main patch, if you'd prefer, but it 
seems more straightforward to include it.

>>> + else if (declarator->declarator->kind == cdk_pointer)
>>> + error_at (DECL_SOURCE_LOCATION (xobj_parm),
>>> + /* "a pointer to function type cannot "? */
>>> + "a function pointer type cannot "
>>> + "have an explicit object parameter");
>>
>> "pointer to function type", yes.
>>
>>> + /* The locations being used here are probably not correct. */
>>
>> Why not?
> 
> I threw them in just so I could call inform, but it doesn't feel like
> the messages should be pointing at the parameter, but rather at the
> full type declaration. When I put those locations in I wasn't sure how
> to get the full declaration location, and I'm still not 100% confident
> in how to do it, so I just threw them in and moved on.

That would be more precise, but I think it's actually preferable for the 
inform to have the same location as the previous error to avoid 
redundant quoting of the source.

>> Let's clear xobj_parm after giving an error in the TYPENAME case
> 
> I don't like the spirit of this very much, whats your reasoning for
> this? We're nearly at the end of the scope where it is last used, I
> think it would be more unclear if we suddenly set it to NULL_TREE near
> the end. It raises the question of whether that assignment actually
> does anything, or if we are just trying to indicate that it isn't being
> used anymore, but I already made sure to declare it in the deepest
> scope possible. That much should be sufficient for indicating it's
> usage, no?

Hmm, I think I poked at that and changed my mind, but forgot to delete 
the suggestion.  Never mind.

>>> if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
>>> || DECL_STATIC_FUNCTION_P (decl))
>>
>> I think this can just be if (DECL_OBJECT_MEMBER_FUNC_P (decl)).
> 
> Alright, and going forward I'll try to make more changes that are
> consistent with this one. With that said I'm not sure it can, but I'll
> take a close look and if you're right I'll make that change.
> 
>>> if (TREE_CODE (fntype) == METHOD_TYPE)
>>> ctype = TYPE_METHOD_BASETYPE (fntype);
>>> + else if (DECL_XOBJ_MEMBER_FUNC_P (decl1))
>>> + ctype = DECL_CONTEXT (decl1);
>>
>> All of this can be
>>
>> if (DECL_CLASS_SCOPE_P (decl1))
>> ctype = DECL_CONTEXT (decl1);
>>
>> I think I'm going to go ahead and clean that up now.
> 
> Sounds good to me, a lot of this stuff needs small cleanups and I'm
> just concerned about making them too much.

My cleanup of the ctype logic is in now.

>>> + /* Error reporting here is a little awkward, if the type of the
>>> + object parameter is deduced, we should tell them the lambda
>>> + is effectively already const, or to make the param const if it is
>>> + not, but if it is deduced and taken by value shouldn't we say
>>> + that it's taken by copy and won't mutate?
>>> + Seems right to me, but it's a little strange. */
>>
>> I think just omit the inform if dependent_type_p.
> 
> Maybe I don't understand what a dependent type is as well as I thought,
> but doesn't this defeat every useful case? The most common being an
> xobj parameter of lambda type, which will always be deduced. Unless a
> template parameter does not count as a dependent type, which is not
> something I've ever thought about before.

No, you're right.  A template parameter is certainly dependent.  I think 
the informs are fine as they are.

> Mildly related, a lot of the stuff I hacked together with multiple
> levels of accessing macros and predicates was due to not being able to
> find a solution for what I needed. I think we would highly benefit from
> better documentation of the accessors and predicates. I believe I've
> seen some that appear to be duplicates, and some where they don't
> appear to be implemented properly or match their description. If there
> is such a document please direct me to it as I have spent an hour or so
> each time I stumble on one of these problems.
> 
> In the section I wrote in add_method I spent I think about 3 hours
> trying to figure out what combination of predicates and accessing
> macros was correct for what I was trying to do. It gets pretty
> convoluted to traverse, especially when the implementations of them are
> rather involved. Finding non_reference was a big help for example, and
> I stumbled across it by pure luck.
> 
> If we don't have this kind of documentation anywhere yet, I'm happy to
> work on it next. I've spent a lot of time looking at it all so I feel
> like I have a DECENT grasp on some of it. I think it would greatly
> enhance the status quo, and might even help us figure out what needs to
> be cleaned up, removed, replaced, etc.
> 
> Side side note, if said document does exist already, I either need to
> learn how to search the wiki more effectively or it needs some sort of
> improvement.

There isn't really such a document; the comments are the main internals 
documentation, such as it is.  I wonder about reorganizing cp-tree.h 
rather than creating a separate document that is likely to go out of 
date more quickly?

Jason

On Wednesday, November 22nd, 2023 at 2:38 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/22/23 15:46, waffl3x wrote:
> 
> > On Tuesday, November 21st, 2023 at 8:22 PM, Jason Merrill jason@redhat.com wrote:
> > 
> > > On 11/21/23 08:04, waffl3x wrote:
> > > 
> > > > /* Nonzero for FUNCTION_DECL means that this decl is a non-static
> > > > - member function. */
> > > > + member function, use DECL_IOBJ_MEMBER_FUNC_P instead. */
> > > > #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
> > > > (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> > > > 
> > > > +/* Nonzero for FUNCTION_DECL means that this decl is an implicit object
> > > > + member function. */
> > > > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> > > > + (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> > > 
> > > I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than
> > > add a new, equivalent one. And then go through all the current uses of
> > > the old macro to decide whether they mean IOBJ or OBJECT.
> > 
> > I figure it would be easier to make that transition if there's a clear
> > line between old versus new. To be clear, my intention is for the old
> > macro to be removed once all the uses of it are changed over to the new
> > macro. I can still remove it for the patch if you like but having both
> > and removing the old one later seems better to me.
> 
> 
> Hmm, I think changing all the uses is a necessary part of this change.
> I suppose it could happen before the main patch, if you'd prefer, but it
> seems more straightforward to include it.
> 
> > > > + else if (declarator->declarator->kind == cdk_pointer)
> > > > + error_at (DECL_SOURCE_LOCATION (xobj_parm),
> > > > + /* "a pointer to function type cannot "? */
> > > > + "a function pointer type cannot "
> > > > + "have an explicit object parameter");
> > > 
> > > "pointer to function type", yes.
> > > 
> > > > + /* The locations being used here are probably not correct. */
> > > 
> > > Why not?
> > 
> > I threw them in just so I could call inform, but it doesn't feel like
> > the messages should be pointing at the parameter, but rather at the
> > full type declaration. When I put those locations in I wasn't sure how
> > to get the full declaration location, and I'm still not 100% confident
> > in how to do it, so I just threw them in and moved on.
> 
> 
> That would be more precise, but I think it's actually preferable for the
> inform to have the same location as the previous error to avoid
> redundant quoting of the source.

Yeah that makes sense, I'll revise the comment with that rationale and
we can maybe revisit it later.

> > > Let's clear xobj_parm after giving an error in the TYPENAME case
> > 
> > I don't like the spirit of this very much, whats your reasoning for
> > this? We're nearly at the end of the scope where it is last used, I
> > think it would be more unclear if we suddenly set it to NULL_TREE near
> > the end. It raises the question of whether that assignment actually
> > does anything, or if we are just trying to indicate that it isn't being
> > used anymore, but I already made sure to declare it in the deepest
> > scope possible. That much should be sufficient for indicating it's
> > usage, no?
> 
> 
> Hmm, I think I poked at that and changed my mind, but forgot to delete
> the suggestion. Never mind.

Perfect.

> > > > if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
> > > > || DECL_STATIC_FUNCTION_P (decl))
> > > 
> > > I think this can just be if (DECL_OBJECT_MEMBER_FUNC_P (decl)).
> > 
> > Alright, and going forward I'll try to make more changes that are
> > consistent with this one. With that said I'm not sure it can, but I'll
> > take a close look and if you're right I'll make that change.
> > 
> > > > if (TREE_CODE (fntype) == METHOD_TYPE)
> > > > ctype = TYPE_METHOD_BASETYPE (fntype);
> > > > + else if (DECL_XOBJ_MEMBER_FUNC_P (decl1))
> > > > + ctype = DECL_CONTEXT (decl1);
> > > 
> > > All of this can be
> > > 
> > > if (DECL_CLASS_SCOPE_P (decl1))
> > > ctype = DECL_CONTEXT (decl1);
> > > 
> > > I think I'm going to go ahead and clean that up now.
> > 
> > Sounds good to me, a lot of this stuff needs small cleanups and I'm
> > just concerned about making them too much.
> 
> 
> My cleanup of the ctype logic is in now.

I'll make sure to base the final patch off a newer commit then, I don't
think I'll do that right now because it takes a lot of time for me. I
still haven't gotten used to all the workflows with git so doing
anything with it takes a lot out of me. Also, I would have to rerun the
testsuite on the newer commit so I can get an accurate baseline which
takes a few hours.

> > > > + /* Error reporting here is a little awkward, if the type of the
> > > > + object parameter is deduced, we should tell them the lambda
> > > > + is effectively already const, or to make the param const if it is
> > > > + not, but if it is deduced and taken by value shouldn't we say
> > > > + that it's taken by copy and won't mutate?
> > > > + Seems right to me, but it's a little strange. */
> > > 
> > > I think just omit the inform if dependent_type_p.
> > 
> > Maybe I don't understand what a dependent type is as well as I thought,
> > but doesn't this defeat every useful case? The most common being an
> > xobj parameter of lambda type, which will always be deduced. Unless a
> > template parameter does not count as a dependent type, which is not
> > something I've ever thought about before.
> 
> 
> No, you're right. A template parameter is certainly dependent. I think
> the informs are fine as they are.

Okay sounds good, I still have to clean up in there for the other
reasons you noted, and because it just doesn't behave the way I want in
some cases still.

I think the logic I want is probably something like, is dependent,
unless it's a type template parameter. I'll figure it out, probably.

> > Mildly related, a lot of the stuff I hacked together with multiple
> > levels of accessing macros and predicates was due to not being able to
> > find a solution for what I needed. I think we would highly benefit from
> > better documentation of the accessors and predicates. I believe I've
> > seen some that appear to be duplicates, and some where they don't
> > appear to be implemented properly or match their description. If there
> > is such a document please direct me to it as I have spent an hour or so
> > each time I stumble on one of these problems.
> > 
> > In the section I wrote in add_method I spent I think about 3 hours
> > trying to figure out what combination of predicates and accessing
> > macros was correct for what I was trying to do. It gets pretty
> > convoluted to traverse, especially when the implementations of them are
> > rather involved. Finding non_reference was a big help for example, and
> > I stumbled across it by pure luck.
> > 
> > If we don't have this kind of documentation anywhere yet, I'm happy to
> > work on it next. I've spent a lot of time looking at it all so I feel
> > like I have a DECENT grasp on some of it. I think it would greatly
> > enhance the status quo, and might even help us figure out what needs to
> > be cleaned up, removed, replaced, etc.
> > 
> > Side side note, if said document does exist already, I either need to
> > learn how to search the wiki more effectively or it needs some sort of
> > improvement.
> 
> 
> There isn't really such a document; the comments are the main internals
> documentation, such as it is. I wonder about reorganizing cp-tree.h
> rather than creating a separate document that is likely to go out of
> date more quickly?
> 
> Jason

Yeah if it gets out of sync it's a problem, I'm not a big fan of
generating documentation from source usually, but perhaps this is a
good case for it. Annotating the macros and predicates with tags to
make it more searchable. The tags being the actual C++ keywords or
whatnot.

Obviously there's not too much time to think about it right now so I'll
just leave it at that for the moment, but some sort of mechanism like
this to aide in organization, de-duplication, and search-ability would
be the right way forward IMO. It will be less likely to get to a state
where it's in a dire need of reorganization that way.

One last thing on that, I've noticed there appears to still be some
pollution of gcc/tree.h with C++ constructs. Indeed, I checked really
quick just now and spotted a predicate for relating to virtual
functions/inheritance. I recognize that's a huge task to fully clean up
so I understand why it's still the case, but that should probably be
tackled at the same time.

/* Nonzero means that the derivation chain is via a `virtual' declaration.  */
#define BINFO_VIRTUAL_P(NODE) (TREE_BINFO_CHECK (NODE)->base.static_flag)

Back to work, thanks for the quick reply.

Alex

> > > > /* Nonzero for FUNCTION_DECL means that this decl is a non-static
> > > > - member function. */
> > > > + member function, use DECL_IOBJ_MEMBER_FUNC_P instead. */
> > > > #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
> > > > (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> > > > 
> > > > +/* Nonzero for FUNCTION_DECL means that this decl is an implicit object
> > > > + member function. */
> > > > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> > > > + (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> > > 
> > > I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than
> > > add a new, equivalent one. And then go through all the current uses of
> > > the old macro to decide whether they mean IOBJ or OBJECT.
> > 
> > I figure it would be easier to make that transition if there's a clear
> > line between old versus new. To be clear, my intention is for the old
> > macro to be removed once all the uses of it are changed over to the new
> > macro. I can still remove it for the patch if you like but having both
> > and removing the old one later seems better to me.
> 
> 
> Hmm, I think changing all the uses is a necessary part of this change.
> I suppose it could happen before the main patch, if you'd prefer, but it
> seems more straightforward to include it.
> 

I had meant to reply to this as well but forgot, I agree that it's
likely necessary but I've only been changing them as I come across
things that don't work right rather than trying to evaluate them
through the code. Making changes to them without having a test case
that demonstrates that the case is definitely being handled incorrectly
is risky, especially for me since I don't have a full understanding of
the code base. I would rather only change ones that are evidently
wrong, and defer the rest to someone else who knows the code base
better.

With that said, I have been neglecting replacing uses of the old macro,
but I now realize that's just creating more work for whoever is
evaluating the rest of them. Going forward I will make sure I replace
the old macro when I am fairly certain it should be.

Alex

> and this in tsubst_lambda_expr that assumes iobj:
> 
> /* Fix the type of 'this'. */
> fntype = build_memfn_type (fntype, type,
> type_memfn_quals (fntype),
> type_memfn_rqual (fntype));

Unfortunately, putting a condition on this had some unforeseen
consequences. I've been working on this about 8 hours today and I'm a
little defeated after discovering this.

commit 39ade88fa1632c659c5c4ed065fa2b62d16a8670
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Jan 24 15:29:35 2023 -0500

    c++: static lambda in template [PR108526]
    
    tsubst_lambda_expr uses build_memfn_type to build a METHOD_TYPE for the new
    lamba op().  This is not what we want for a C++23 static op(), but since we
    also use that METHOD_TYPE to communicate the closure type down to
    tsubst_function_decl, let's wait and turn it back at that point.
    
            PR c++/108526
    
    gcc/cp/ChangeLog:
    
    gcc/cp/ChangeLog:
    
            * pt.cc (tsubst_function_decl): Handle static lambda.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp23/static-operator-call5.C: New test.

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 2a4d03c5e47..51fc246ed71 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -14306,6 +14306,11 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
   tree ctx = closure ? closure : DECL_CONTEXT (t);
   bool member = ctx && TYPE_P (ctx);
 
+  /* If this is a static lambda, remove the 'this' pointer added in
+     tsubst_lambda_expr now that we know the closure type.  */
+  if (lambda_fntype && DECL_STATIC_FUNCTION_P (t))
+    lambda_fntype = static_fn_type (lambda_fntype);
+

I discovered this when I decided to try a static lambda to see if that
would help me narrow down my current problem. I was shocked to find out
it exhibited the exact same ICE I was trying to fix. So I was going to
go undo my changes one by one to see what it was, thankfully this was
the first one I tried, I undid the condition I put on it, and the crash
was gone.

Anyway, this has been my whole day so far, I am going to have to look
deeper to decide how exactly I fix this because I don't think this hack
that is in place at the moment is the right way to do it. The first
idea that comes to mind is modifying the decl_context of the call
operator, but I'm really not sure. I'm just going to take a break, eat
some pizza, and come back to this hopefully less defeated.

Alex

Okay within about half an hour today I think I've figured out what the
main issue is for my problem. For the static lambda case, while I don't
like the hack that's currently present, I reckon we just leave it as it
is, as it works just fine right now. For my issue, I'm not totally sure
how to tackle it, but here's what I've identified.

build_memfn_type calls into build_method_type_directly and that takes
the main variant of the passed in basetype and sets it as the
TYPE_METHOD_BASETYPE.

TYPE_METHOD_BASETYPE (t) = TYPE_MAIN_VARIANT (basetype);

So naively just bypassing build_memfn_type is not going to work, I'm
not totally sure how to solve this though. (Below)

/* Fix the type of 'this'.  */
      fntype = build_memfn_type (fntype, type,
                                   type_memfn_quals (fntype),
                                   type_memfn_rqual (fntype));

Making tree nodes in general is something I'm not very familiar with,
and struggled with every time I tried to read any of the code. So for
now I'm going to hope that the next block (below) handles everything
properly as far as creating a new node and substitution goes.

      tree inst = (oldtmpl
                   ? tsubst_template_decl (oldtmpl, args, complain,
                                           fntype, tparms)
                   : tsubst_function_decl (oldfn, args, complain, fntype));

And I'll just see if setting the decl_context of inst to type does the
trick. I guess I forgot to elaborate that tsubst_function_decl uses the
decl_context of t on the second, which ends up having template-info
attached to it. As you might have been able to deduce from what I've
said so far, I suspect that it's because I'm not setting it properly
when the function_decl is substituted into. I don't really know where
the right place to do that is though, or really why it's not happening
already, so I'm just gonna poke around, try some things, and clean it
up later if it's messy.

That's my quick update, hopefully this gets me somewhere finally.

Alex

On Thursday, November 23rd, 2023 at 11:49 PM, waffl3x <waffl3x@protonmail.com> wrote:


> 
> 
> > and this in tsubst_lambda_expr that assumes iobj:
> 
> > /* Fix the type of 'this'. */
> > fntype = build_memfn_type (fntype, type,
> > type_memfn_quals (fntype),
> > type_memfn_rqual (fntype));
> 
> 
> Unfortunately, putting a condition on this had some unforeseen
> consequences. I've been working on this about 8 hours today and I'm a
> little defeated after discovering this.
> 
> commit 39ade88fa1632c659c5c4ed065fa2b62d16a8670
> Author: Jason Merrill jason@redhat.com
> 
> Date: Tue Jan 24 15:29:35 2023 -0500
> 
> c++: static lambda in template [PR108526]
> 
> tsubst_lambda_expr uses build_memfn_type to build a METHOD_TYPE for the new
> lamba op(). This is not what we want for a C++23 static op(), but since we
> also use that METHOD_TYPE to communicate the closure type down to
> tsubst_function_decl, let's wait and turn it back at that point.
> 
> PR c++/108526
> 
> gcc/cp/ChangeLog:
> 
> gcc/cp/ChangeLog:
> 
> * pt.cc (tsubst_function_decl): Handle static lambda.
> 
> gcc/testsuite/ChangeLog:
> 
> * g++.dg/cpp23/static-operator-call5.C: New test.
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 2a4d03c5e47..51fc246ed71 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -14306,6 +14306,11 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
> tree ctx = closure ? closure : DECL_CONTEXT (t);
> bool member = ctx && TYPE_P (ctx);
> 
> + /* If this is a static lambda, remove the 'this' pointer added in
> + tsubst_lambda_expr now that we know the closure type. */
> + if (lambda_fntype && DECL_STATIC_FUNCTION_P (t))
> + lambda_fntype = static_fn_type (lambda_fntype);
> +
> 
> I discovered this when I decided to try a static lambda to see if that
> would help me narrow down my current problem. I was shocked to find out
> it exhibited the exact same ICE I was trying to fix. So I was going to
> go undo my changes one by one to see what it was, thankfully this was
> the first one I tried, I undid the condition I put on it, and the crash
> was gone.
> 
> Anyway, this has been my whole day so far, I am going to have to look
> deeper to decide how exactly I fix this because I don't think this hack
> that is in place at the moment is the right way to do it. The first
> idea that comes to mind is modifying the decl_context of the call
> operator, but I'm really not sure. I'm just going to take a break, eat
> some pizza, and come back to this hopefully less defeated.
> 
> Alex

OKAY, I figured out SOMETHING, I think this should be fine. As noted in
the comments, this might be a better way of handling the static lambda
case too. This is still a nasty hack so it should probably be done
differently, but I question if making a whole new fntype node in
tsubst_lambda_expr makes sense. On the other hand, maybe there will be
problems if a lambda is already instantiated? I'm not sure, mutating
things this way makes me uneasy though.

I don't like that pt.cc feels like it has a ton of hidden mutations,
it's really hard to follow through it. Would you agree it's in need for
cleanup or am I just not experienced enough in this area yet?

I still have to write good tests for this case and move the error
handling for unrelated object types into here, but my gut tells me this
issue should be fixed now.

Regarding the error handling, I just had a thought about it, I have a
hunch it definitely needs to go in tsubst_template_decl or
tsubst_function_decl. There might need to be more changes to determine
the actual type of the lambda in there, but everything else I've done
changes the implicit object argument to be treated more like a regular
argument, doing error handling for the object type in
tsubst_lambda_expr would be inconsistent with that.

Also, before I forget, you were totally right about just using
dependent_type_p in cp_parser_lambda_declarator_opt. It really is just
that simple.

The code below is not necessarily final, but I wanted to get your
thoughts about doing it this way given my concerns written above. Now
that this problem is out of the way, or at least almost out of the way,
I hopefully can get a version pushed out today.

Alex

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 86c95b278ba..ea2db5c0c9d 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -14407,14 +14407,20 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
       gen_tmpl = NULL_TREE;
       argvec = NULL_TREE;
     }
-
+  /* We hack TYPE_METHOD_BASETYPE onto xobj member functions in
+     tsubst_lambda_expr to get the proper closure type here.  */
   tree closure = (lambda_fntype ? TYPE_METHOD_BASETYPE (lambda_fntype)
                   : NULL_TREE);
   tree ctx = closure ? closure : DECL_CONTEXT (t);
   bool member = ctx && TYPE_P (ctx);
 
   /* If this is a static lambda, remove the 'this' pointer added in
-     tsubst_lambda_expr now that we know the closure type.  */
+     tsubst_lambda_expr now that we know the closure type.
+     I suspect that we can just carry this information down in
+     TYPE_METHOD_BASETYPE without building the full method type in
+     tsubst_lambda_expr.  This wouldn't be ideal but neither is this.
+     Since that field isn't used for anything for static or xobj member
+     functions, it should be fine to do that.  */
   if (lambda_fntype && DECL_STATIC_FUNCTION_P (t))
     lambda_fntype = static_fn_type (lambda_fntype);
 
@@ -14490,12 +14496,12 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
     DECL_NAME (r) = make_conv_op_name (TREE_TYPE (type));
 
   tree parms = DECL_ARGUMENTS (t);
-  if (closure && !DECL_STATIC_FUNCTION_P (t))
+  if (closure && DECL_IOBJ_MEMBER_FUNC_P (t))
     parms = DECL_CHAIN (parms);
   parms = tsubst (parms, args, complain, t);
   for (tree parm = parms; parm; parm = DECL_CHAIN (parm))
     DECL_CONTEXT (parm) = r;
-  if (closure && !DECL_STATIC_FUNCTION_P (t))
+  if (closure && DECL_IOBJ_MEMBER_FUNC_P (t))
     {
       tree tparm = build_this_parm (r, closure, type_memfn_quals (type));
       DECL_NAME (tparm) = closure_identifier;
@@ -19491,9 +19497,16 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
       cp_evaluated ev;
 
       /* Fix the type of 'this'.  */
-      fntype = build_memfn_type (fntype, type,
-				 type_memfn_quals (fntype),
-				 type_memfn_rqual (fntype));
+      if (DECL_IOBJ_MEMBER_FUNC_P (oldfn))
+	fntype = build_memfn_type (fntype, type,
+				   type_memfn_quals (fntype),
+				   type_memfn_rqual (fntype));
+      /* We don't use this field anywhere else for xobj member functions, and
+	 we need a way to pass this information down.  Theres some really
+	 convoluted stuff going on unfortunately, and I can only begin to
+	 unravel it all.   */
+      if (DECL_XOBJ_MEMBER_FUNC_P (oldfn))
+	TYPE_METHOD_BASETYPE (fntype) = type;
       tree inst = (oldtmpl
                    ? tsubst_template_decl (oldtmpl, args, complain,
                                            fntype, tparms)

On 11/24/23 01:49, waffl3x wrote:
>> and this in tsubst_lambda_expr that assumes iobj:
>>
>> /* Fix the type of 'this'. */
>> fntype = build_memfn_type (fntype, type,
>> type_memfn_quals (fntype),
>> type_memfn_rqual (fntype));
> 
> Unfortunately, putting a condition on this had some unforeseen
> consequences. I've been working on this about 8 hours today and I'm a
> little defeated after discovering this.
> 
> commit 39ade88fa1632c659c5c4ed065fa2b62d16a8670
> Author: Jason Merrill <jason@redhat.com>
> Date:   Tue Jan 24 15:29:35 2023 -0500
> 
>      c++: static lambda in template [PR108526]
>      
>      tsubst_lambda_expr uses build_memfn_type to build a METHOD_TYPE for the new
>      lamba op().  This is not what we want for a C++23 static op(), but since we
>      also use that METHOD_TYPE to communicate the closure type down to
>      tsubst_function_decl, let's wait and turn it back at that point.
>      
>              PR c++/108526
>      
>      gcc/cp/ChangeLog:
>      
>      gcc/cp/ChangeLog:
>      
>              * pt.cc (tsubst_function_decl): Handle static lambda.
>      
>      gcc/testsuite/ChangeLog:
>      
>              * g++.dg/cpp23/static-operator-call5.C: New test.
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 2a4d03c5e47..51fc246ed71 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -14306,6 +14306,11 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
>     tree ctx = closure ? closure : DECL_CONTEXT (t);
>     bool member = ctx && TYPE_P (ctx);
>   
> +  /* If this is a static lambda, remove the 'this' pointer added in
> +     tsubst_lambda_expr now that we know the closure type.  */
> +  if (lambda_fntype && DECL_STATIC_FUNCTION_P (t))
> +    lambda_fntype = static_fn_type (lambda_fntype);

Ah, right.  So the simplest thing would be to do the same thing here for 
xob lambdas.

Jason

I don't think so, I'm concerned that it might eat the xobj parameter if
we do it that way. Besides, all we want is the lambda type anyway, I
don't want to build a whole new node just to do that. Upon further
consideration I think my solution does work and shouldn't cause
problems. The field that I'm using just doesn't get used anywhere else
for function_type nodes, and we create a new node in
tsubst_function_decl so we don't rely on that field anywhere after it's
used in tsubst_function_decl. I think I will null out the member,
consuming it, so that is clearly communicated.

I am still having issues trying to figure out where exactly to emit
errors from, it's really unclear how this stuff works during template
instantiation. I tried emitting conditionally based on the complain
parameter, but that seemed to suppress errors at all times. While at
the same time, I seemed to observe emitting errors unconditionally
(correctly) not not displaying the error when another candidate was
selected. This leads me to believe that there's already a mechanism for
discarding errors that occur during substitution. I don't really know
what to do for it, but surely I will figure it out now that I'm asking
for help, as tradition dictates.

The other problem I'm having is

auto f0 = [n = 5, &m](this auto const&){ n = 10; };
This errors just fine, the lambda is unconditionally const so
LAMBDA_EXPR_MUTABLE_P flag is set for the closure.

This on the other hand does not. The constness of the captures depends
on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
here.
auto f1 = [n = 5](this auto&& self){ n = 10; };
as_const(f1)();

I don't know the right way to solve this, I haven't even really tracked
down where the tree is built up. At the moment I think that hacking
something into instantiate_body to make them const is the right
decision, as the type of the closure shouldn't be changing when
instantiating it's call operator template. I'm not sure this is really
actually the right call though because it's another thing that feels
very much like a hack. As I near completion of the patch it feels like
more and more of the solutions I'm finding are hacks, and I don't know
if it's just a consequence of old assumptions or if I'm really just
reaching for the easy(ier) solutions way too quickly now.

These problems are the last few that I have and they are taking me way
more time than I am comfortable with.

I was going to close out this message there but I just realized why
exactly you think erroring in instantiate_body is too late, it's
because at that point an error is an error, while if we error a little
bit earlier during substitution it's not a hard error. I'm glad I
realized this now, because I am much more confident in how to implement
the errors for unrelated type now. I'm still a little confused on what
the exact protocol for emitting errors at this stage is, as there
aren't many explicit errors written in. It seems to me like the errors
are supposed to be emitted when calling back into non-template stages
of the compiler with substituted types. It also seems like that hasn't
always been strictly followed, and I hate to contribute to code debt
but I'm not sure how I would do it in this case, nor if I actually have
a valid understanding of how this all works.

Given my above realization, I'm now more confident in figuring out how
exactly to implement the errors for unrelated types. I guess I'm rubber
ducking a little bit in these e-mails. On the other hand, if the
conclusions I'm forming are incorrect, it's probably still helpful to
know the journey I took to get to them. I'm thinking it's probably
better I still include my initial concerns even though I feel like I
know the answer now.

One side note before I close up here, I don't like when *_P macros are
used to set flags. Is this
something else we should clean up in the future? I'm beginning to
wonder if an overhaul that gets rid of the macros from the public
interface is a good idea. I'm reluctant to suggest that as I've really
warmed up to the macros a lot. They are used in a consistent and easy
to understand way which is highly unlike the bad uses of macros that
I've seen before that really obscure what's actually going on. But they
are still macros, so maybe moving away from them is the right call,
especially since there has started to be a mix-up of macros and
functions for the same purposes. I'm mildly of the opinion that only
one style should be used (in the public interface) because mixing them
causes confusion, it did for me anyway. Perhaps I should open a thread
on the general mail list and see what others think, get some input
before I decide which direction to go with it. To be clear, when I say
getting rid of macros, I want to emphasize I mean only in the public
interface, I don't see any value in getting rid of macros under the
hood as the way the checking macros are implemented is already really
good and works. It would only cause problems to try to move away from
that. I think I'll probably start to mess around with this idea as soon
as this patch is done.

That unrelated rambling aside, I will get back to work and try my best
to finish the errors for unrelated types today. It feels kind of bad to
work so hard on this since I've noticed that it's almost impossible to
coerce the case other than overload resolution when taking the address
of a lambdas operator. But that's not an excuse to leave a hole in the
compiler so, it's still gotta get done. I don't think I will get
rejection of mutation of captures done, especially since I'm uncertain
of what the right way to do it is, but maybe I'll work fast.


On Saturday, November 25th, 2023 at 10:32 AM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/24/23 01:49, waffl3x wrote:
> 
> > > and this in tsubst_lambda_expr that assumes iobj:
> > > 
> > > /* Fix the type of 'this'. */
> > > fntype = build_memfn_type (fntype, type,
> > > type_memfn_quals (fntype),
> > > type_memfn_rqual (fntype));
> > 
> > Unfortunately, putting a condition on this had some unforeseen
> > consequences. I've been working on this about 8 hours today and I'm a
> > little defeated after discovering this.
> > 
> > commit 39ade88fa1632c659c5c4ed065fa2b62d16a8670
> > Author: Jason Merrill jason@redhat.com
> > Date: Tue Jan 24 15:29:35 2023 -0500
> > 
> > c++: static lambda in template [PR108526]
> > 
> > tsubst_lambda_expr uses build_memfn_type to build a METHOD_TYPE for the new
> > lamba op(). This is not what we want for a C++23 static op(), but since we
> > also use that METHOD_TYPE to communicate the closure type down to
> > tsubst_function_decl, let's wait and turn it back at that point.
> > 
> > PR c++/108526
> > 
> > gcc/cp/ChangeLog:
> > 
> > gcc/cp/ChangeLog:
> > 
> > * pt.cc (tsubst_function_decl): Handle static lambda.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > * g++.dg/cpp23/static-operator-call5.C: New test.
> > 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 2a4d03c5e47..51fc246ed71 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -14306,6 +14306,11 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
> > tree ctx = closure ? closure : DECL_CONTEXT (t);
> > bool member = ctx && TYPE_P (ctx);
> > 
> > + /* If this is a static lambda, remove the 'this' pointer added in
> > + tsubst_lambda_expr now that we know the closure type. */
> > + if (lambda_fntype && DECL_STATIC_FUNCTION_P (t))
> > + lambda_fntype = static_fn_type (lambda_fntype);
> 
> 
> Ah, right. So the simplest thing would be to do the same thing here for
> xob lambdas.
> 
> Jason

On 11/24/23 20:14, waffl3x wrote:
> OKAY, I figured out SOMETHING, I think this should be fine. As noted in
> the comments, this might be a better way of handling the static lambda
> case too. This is still a nasty hack so it should probably be done
> differently, but I question if making a whole new fntype node in
> tsubst_lambda_expr makes sense. On the other hand, maybe there will be
> problems if a lambda is already instantiated? I'm not sure, mutating
> things this way makes me uneasy though.

A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is that 
all equivalent FUNCTION_TYPE share the same tree node (through 
type_hash_canon), so you're messing with the type of unrelated functions 
at the same time.  I think it's better to stick with the way static 
lambdas are handled.

> I don't like that pt.cc feels like it has a ton of hidden mutations,
> it's really hard to follow through it. Would you agree it's in need for
> cleanup or am I just not experienced enough in this area yet?

I'm sure there are things that could use cleaning up, but I'm not 
thinking of anything specific offhand.  Any particular examples?

> Regarding the error handling, I just had a thought about it, I have a
> hunch it definitely needs to go in tsubst_template_decl or
> tsubst_function_decl. There might need to be more changes to determine
> the actual type of the lambda in there, but everything else I've done
> changes the implicit object argument to be treated more like a regular
> argument, doing error handling for the object type in
> tsubst_lambda_expr would be inconsistent with that.

But the rule about unrelated type is specific to lambdas, so doing it in 
tsubst_lambda_expr seems preferable to adding a lambda special case to 
generic code.

> The other problem I'm having is
> 
> auto f0 = [n = 5, &m](this auto const&){ n = 10; };
> This errors just fine, the lambda is unconditionally const so
> LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
> 
> This on the other hand does not. The constness of the captures depends
> on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
> here.
> auto f1 = [n = 5](this auto&& self){ n = 10; };
> as_const(f1)();

That sounds correct, why would this be an error?

The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P, 
it depends on the type of the object parameter, which in this case is 
non-const, so so are the captures.

> I was going to close out this message there but I just realized why
> exactly you think erroring in instantiate_body is too late, it's
> because at that point an error is an error, while if we error a little
> bit earlier during substitution it's not a hard error. I'm glad I
> realized this now, because I am much more confident in how to implement
> the errors for unrelated type now. I'm still a little confused on what
> the exact protocol for emitting errors at this stage is, as there
> aren't many explicit errors written in. It seems to me like the errors
> are supposed to be emitted when calling back into non-template stages
> of the compiler with substituted types. It also seems like that hasn't
> always been strictly followed, and I hate to contribute to code debt
> but I'm not sure how I would do it in this case, nor if I actually have
> a valid understanding of how this all works.

Most errors that could occur in template substitution are guarded by if 
(complain & tf_error) so that they aren't emitted during deduction 
substitution.  It's better if those are in code that's shared with the 
non-template case, but sometimes that isn't feasible.

> One side note before I close up here, I don't like when *_P macros are
> used to set flags. Is this something else we should clean up in the future?

I don't think so, a wholesale renaming would just confuse existing 
developers.

> I'm beginning to
> wonder if an overhaul that gets rid of the macros from the public
> interface is a good idea. I'm reluctant to suggest that as I've really
> warmed up to the macros a lot. They are used in a consistent and easy
> to understand way which is highly unlike the bad uses of macros that
> I've seen before that really obscure what's actually going on. But they
> are still macros, so maybe moving away from them is the right call,
> especially since there has started to be a mix-up of macros and
> functions for the same purposes. I'm mildly of the opinion that only
> one style should be used (in the public interface) because mixing them
> causes confusion, it did for me anyway. Perhaps I should open a thread
> on the general mail list and see what others think, get some input
> before I decide which direction to go with it. To be clear, when I say
> getting rid of macros, I want to emphasize I mean only in the public
> interface, I don't see any value in getting rid of macros under the
> hood as the way the checking macros are implemented is already really
> good and works. It would only cause problems to try to move away from
> that. I think I'll probably start to mess around with this idea as soon
> as this patch is done.

There has been some movement from macros toward inline functions since 
the switch to C++, but that also has complications with 
const-correctness, declaration order, and bit-fields.  It's probably 
good to use inlines instead of larger macros when feasible, but the 
accessors should probably stay macros.

Jason

On Sunday, November 26th, 2023 at 2:30 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/24/23 20:14, waffl3x wrote:
> 
> > OKAY, I figured out SOMETHING, I think this should be fine. As noted in
> > the comments, this might be a better way of handling the static lambda
> > case too. This is still a nasty hack so it should probably be done
> > differently, but I question if making a whole new fntype node in
> > tsubst_lambda_expr makes sense. On the other hand, maybe there will be
> > problems if a lambda is already instantiated? I'm not sure, mutating
> > things this way makes me uneasy though.
> 
> 
> A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is that
> all equivalent FUNCTION_TYPE share the same tree node (through
> type_hash_canon), so you're messing with the type of unrelated functions
> at the same time. I think it's better to stick with the way static
> lambdas are handled.

Yes, that was my concern as well, without even thinking about hashing.
I will investigate it deeper but I'm pretty sure TYPE_METHOD_BASETYPE,
while a valid field, is not used at all for function_type nodes. I had
to look into this when looking into the DECL_CONTEXT stuff previously,
so I'm pretty confident in this. Come to think of it, does hashing even
take fields that aren't "supposed" to be set for a node into account?
Well, even so, I can just clear it after it gets used as we just need
it to pass the closure type down. Perhaps I should have led with this,
but as it stands the version that uses TYPE_METHOD_BASETYPE bootstraps
with no regressions. I'll still look deeper but I'm pretty confident in
my decision here, I really don't want to try to unravel what
build_memfn_type does, I would rather find a whole different way of
passing that information down.

> > I don't like that pt.cc feels like it has a ton of hidden mutations,
> > it's really hard to follow through it. Would you agree it's in need for
> > cleanup or am I just not experienced enough in this area yet?
> 
> 
> I'm sure there are things that could use cleaning up, but I'm not
> thinking of anything specific offhand. Any particular examples?

Not at the moment, just after 8 hours of following the execution path
you start to feel like something could be done better. I'm sorry if I
sound bitter but to be honest I kind of am. :^)

I don't think it's going to be my first goal to look at pt.cc for
things to clean up, I honestly didn't want to go through here at all.
Maybe if I look at it for longer I will feel better about it but I want
to make that a task for the future.

> > Regarding the error handling, I just had a thought about it, I have a
> > hunch it definitely needs to go in tsubst_template_decl or
> > tsubst_function_decl. There might need to be more changes to determine
> > the actual type of the lambda in there, but everything else I've done
> > changes the implicit object argument to be treated more like a regular
> > argument, doing error handling for the object type in
> > tsubst_lambda_expr would be inconsistent with that.
> 
> 
> But the rule about unrelated type is specific to lambdas, so doing it in
> tsubst_lambda_expr seems preferable to adding a lambda special case to
> generic code.

No, while that might seem intuitive, the error is not during
instantiation of the closure object's type, it's during instantiation
of the function template. The type of the closure will always be the
type of the closure, but the type of the explicit object parameter can
be different. In the following example we don't go into
tsubst_lambda_expr at all.

template<typename T>
struct S : T {
  using T::operator();
  using s_tag = void;
};

int main()
{
  auto f = [](this auto self){
    if constexpr ( requires{typename decltype(self)::s_tag;} ) {
      return 5;
    }
    else {
      return 10;
    }
  };
  auto s = S{f};

  if (f () != 10)
    __builtin_abort ();
  if (s () != 5)
    __builtin_abort ();
}

I hope this demonstrates that placing the check in tsubst_function_decl
is the correct way to do this.

Now with that out of the way, I do still kind of feel icky about it.
This really is a MASSIVE edge case that will almost never matter. As
far as I know, instantiating explicitly like so... 

auto f = [x = 42](this auto&&) -> int { return x; };
int (*fp)(int&) = &decltype(f)::operator();

...is the only way to coerce the explicit object parameter of the
lambda's call operator into deducing as an unrelated type. Cases that
are not deduced can be caught trivially while parsing the lambda and
are the only reasonable cases where you might have an unrelated type.
Perhaps it might become relevant in the future if a proposal like
https://atomgalaxy.github.io/isocpp-1107/D1107.html ever made it in,
but we aren't there yet. So as it stands, I'm pretty certain that it's
just impossible to instantiate a lambda's call operator with an
unrelated xobj parameter type except for the above case with
address-of. If you can think of another, please do share, but I've
spent a fair amount of time on it and came up with nothing.

Anyway, due to this, I am not currently concerned about the lack of a
diagnostic, and I believe my implementation is the most correct way to
do it. The problem with resolve_address_of_overloaded_function that I
go into below (depending on if you agree that it's a problem) is what
needs to be fixed to allow the current diagnostic to be properly
emitted. As of right now, there is no execution path that I know of
where the diagnostic will be properly emitted.

I go into more detail about this in the comment in
tsubst_function_decl, although I do have to revise it a bit as I wrote
it before I had a discussion with another developer on the correct
behavior of the following case. I previously wondered if the overload
resolution from initializing p1 should result in a hard fault.

template<typename T>
struct S : T {
  using T::operator();
  void operator()(this int&, auto) {}
};

int main()
{
  auto s0 = S{[](this auto&&, auto){}};
  // error, ambiguous
  void (*p0)(int&, int) = &decltype(s0)::operator();

  auto s1 = S{[x = 42](this auto&&, auto){}};
  // SFINAE's, calls S::operator()
  void (*p1)(int&, int) = &decltype(s1)::operator();
}

The wording in [expr.prim.lambda.closure-5] is similar to that in
[dcl.fct-15], and we harness SFINAE in that case, so he concluded that
this case (initializing p1) should also harness SFINAE. Luckily for me,
that is what my implementation already did. The big comment in
tsubst_function_decl does not reflect this conclusion yet so I will
have to revise it a little bit before I can submit v6, but once I do
that I will follow up this e-mail with it.
https://eel.is/c++draft/dcl.fct#15
https://eel.is/c++draft/expr.prim#lambda.closure-5

> > The other problem I'm having is
> > 
> > auto f0 = [n = 5, &m](this auto const&){ n = 10; };
> > This errors just fine, the lambda is unconditionally const so
> > LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
> > 
> > This on the other hand does not. The constness of the captures depends
> > on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
> > here.
> > auto f1 = [n = 5](this auto&& self){ n = 10; };
> > as_const(f1)();
> 
> 
> That sounds correct, why would this be an error?
> 
> The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P,
> it depends on the type of the object parameter, which in this case is
> non-const, so so are the captures.

Oops, I should have just made a less terse example, you missed the
"as_const" in the call to the lambda. The object parameter should be
deduced as const here. I definitely should have made that example
better, my bad.

Anyway, yes I agree that it's -supposed- to depend on the object
parameter, but that isn't the behavior I'm observing right now. Perhaps
you're right that it's not based on the LAMBDA_EXPR_MUTABLE_P flag, I
mostly just assumed that was the case, but it does seem to be based on
the closure object. Now that I'm clear of the previous problem today
will be spent focusing on this bug.

> > I was going to close out this message there but I just realized why
> > exactly you think erroring in instantiate_body is too late, it's
> > because at that point an error is an error, while if we error a little
> > bit earlier during substitution it's not a hard error. I'm glad I
> > realized this now, because I am much more confident in how to implement
> > the errors for unrelated type now. I'm still a little confused on what
> > the exact protocol for emitting errors at this stage is, as there
> > aren't many explicit errors written in. It seems to me like the errors
> > are supposed to be emitted when calling back into non-template stages
> > of the compiler with substituted types. It also seems like that hasn't
> > always been strictly followed, and I hate to contribute to code debt
> > but I'm not sure how I would do it in this case, nor if I actually have
> > a valid understanding of how this all works.
> 
> 
> Most errors that could occur in template substitution are guarded by if
> (complain & tf_error) so that they aren't emitted during deduction
> substitution. It's better if those are in code that's shared with the
> non-template case, but sometimes that isn't feasible.

Yeah, makes sense, but the case going through
resolve_address_of_overloaded_function -> fn_type_unification ->
instantiate_template -> tsubst_decl -> tsubst_function_decl does not
behave super nicely. I tried adding (complain & tf_error) to the
explain_p parameter of fn_type_unification in
resolve_address_of_overloaded_function, but I immediately learned why
that wasn't being handled that way. I think
resolve_address_of_overloaded_function has a number of problems and
should probably use print_z_candidates instead of print_candidates in
the long term. This would actually solve the problem in
tsubst_function_decl where it never emits an error for an unrelated
type, print_z_candidates does call fn_type_unification with true passed
to explain_p. I go into this in detail in the large comment in
tsubst_function_decl (that I have to revise) so I won't waste time
rehashing it here.

Thank you for explaining though, some of the code doesn't reflect the
method you've relayed here (such as an unguarded error_at in
tsubst_function_decl) so I was starting to get confused on what exactly
is supposed to be going on. This clarifies things for me.

> > One side note before I close up here, I don't like when *_P macros are
> > used to set flags. Is this something else we should clean up in the future?
> 
> 
> I don't think so, a wholesale renaming would just confuse existing
> developers.

I don't think changing *_P macros to be immutable, and adding
corresponding *_SET or *_FLAG macros (or functions) would be confusing.
I think it's much more confusing to have the inconsistent behavior.

In case it's not clear, I'm not proposing we change any of the names
here. I disliked the *_P name scheme at first, but as I became familiar
with it I think it's good, especially once I realized that it stands
for predicate. Even at that point though I was pretty sure that
convention should stick as it's not worth the confusion just because a
single developer, let alone a new one (me), doesn't like it. And I like
it now so I absolutely wouldn't propose making that change.

But I do think they should be immutable, this has other benefits anyway
since it will be more clear where we observe state, and where we mutate
it.

> > I'm beginning to
> > wonder if an overhaul that gets rid of the macros from the public
> > interface is a good idea. I'm reluctant to suggest that as I've really
> > warmed up to the macros a lot. They are used in a consistent and easy
> > to understand way which is highly unlike the bad uses of macros that
> > I've seen before that really obscure what's actually going on. But they
> > are still macros, so maybe moving away from them is the right call,
> > especially since there has started to be a mix-up of macros and
> > functions for the same purposes. I'm mildly of the opinion that only
> > one style should be used (in the public interface) because mixing them
> > causes confusion, it did for me anyway. Perhaps I should open a thread
> > on the general mail list and see what others think, get some input
> > before I decide which direction to go with it. To be clear, when I say
> > getting rid of macros, I want to emphasize I mean only in the public
> > interface, I don't see any value in getting rid of macros under the
> > hood as the way the checking macros are implemented is already really
> > good and works. It would only cause problems to try to move away from
> > that. I think I'll probably start to mess around with this idea as soon
> > as this patch is done.
> 
> 
> There has been some movement from macros toward inline functions since
> the switch to C++, but that also has complications with
> const-correctness, declaration order, and bit-fields. It's probably
> good to use inlines instead of larger macros when feasible, but the
> accessors should probably stay macros.
> 
> Jason

Yeah I've noticed the pains with const-correctness, it seems like a
nightmare. However, thats one of the reasons I want to tackle it, I
think fixing and propagating const correctness throughout the code base
would be the biggest thing that could be done to improve it. I have
really poor short term memory so once you start piling on mutations I
have a hard time reasoning about code really quickly.

I see what you mean regarding accessors, trying to access or mutate
bitfields through functions might be way more trouble than it's worth,
especially if we wanted to keep the `SOME_FIELD (some_node) =
new_value` pattern. We would need to return proxy objects and those
just don't always optimize properly in my (limited) experience. I plan
on playing around with some solutions once this patch is done, I'll see
what feels good, what feels bad, whether it's actually viable to change
the accessors into functions, etc.

One developer in IRC expressed that he wished that accessors were
member functions, which I also have kind of wished before. I feel like
it shouldn't be the first refactor though.

I will follow up this e-mail shortly with v6, I just have to clean up
that comment as I said. I don't want to spend too much time on it
because I want to get a proof-of-concept fix for the "lambda capture's
not const" bug today but I'll try to throw it together with a little
more effort than v5.

I do want to note though, despite my claims above that the patch is
bootstrapped and tested without regressions, I did have to make a small
change in tsubst_lambda_expr. When I refactored the changes there I
made a thinko and checked for iobj member function in the condition
before build_memfn_type, completely forgetting about the static lambda
case. I changed it back to !DECL_XOBJ_MEMBER_FUNC_P and ran the 1 test
that failed (static-operator-call5.C) and confirmed that it worked
again, but running the full test suite takes me about an hour and a
half. SO, point is, I'm confident that change broke nothing, but due to
the change technically this version is not regression tested. I am
running a bootstrap since that will finish before I'm done revising the
comment though. Hope this isn't a problem.

I will try to finish up quickly so you can get your eyes on the patch
ASAP.

Alex

On 11/26/23 18:10, waffl3x wrote:
> On Sunday, November 26th, 2023 at 2:30 PM, Jason Merrill <jason@redhat.com> wrote:
>> On 11/24/23 20:14, waffl3x wrote:
>>
>>> OKAY, I figured out SOMETHING, I think this should be fine. As noted in
>>> the comments, this might be a better way of handling the static lambda
>>> case too. This is still a nasty hack so it should probably be done
>>> differently, but I question if making a whole new fntype node in
>>> tsubst_lambda_expr makes sense. On the other hand, maybe there will be
>>> problems if a lambda is already instantiated? I'm not sure, mutating
>>> things this way makes me uneasy though.
>>
>> A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is that
>> all equivalent FUNCTION_TYPE share the same tree node (through
>> type_hash_canon), so you're messing with the type of unrelated functions
>> at the same time. I think it's better to stick with the way static
>> lambdas are handled.
> 
> Yes, that was my concern as well, without even thinking about hashing.
> I will investigate it deeper but I'm pretty sure TYPE_METHOD_BASETYPE,
> while a valid field, is not used at all for function_type nodes. I had
> to look into this when looking into the DECL_CONTEXT stuff previously,
> so I'm pretty confident in this. Come to think of it, does hashing even
> take fields that aren't "supposed" to be set for a node into account?

It doesn't.  The issue is messing with the type of (potentially) a lot 
of different functions.  Even if it doesn't actually break anything, it 
seems like the kind of hidden mutation that you were objecting to.

> Well, even so, I can just clear it after it gets used as we just need
> it to pass the closure type down. Perhaps I should have led with this,
> but as it stands the version that uses TYPE_METHOD_BASETYPE bootstraps
> with no regressions. I'll still look deeper but I'm pretty confident in
> my decision here, I really don't want to try to unravel what
> build_memfn_type does, I would rather find a whole different way of
> passing that information down.

But the existing code already works fine, it's just a question of 
changing the conditions so we handle xob lambdas the same way as static.

>>> Regarding the error handling, I just had a thought about it, I have a
>>> hunch it definitely needs to go in tsubst_template_decl or
>>> tsubst_function_decl. There might need to be more changes to determine
>>> the actual type of the lambda in there, but everything else I've done
>>> changes the implicit object argument to be treated more like a regular
>>> argument, doing error handling for the object type in
>>> tsubst_lambda_expr would be inconsistent with that.
>>
>> But the rule about unrelated type is specific to lambdas, so doing it in
>> tsubst_lambda_expr seems preferable to adding a lambda special case to
>> generic code.
> 
> No, while that might seem intuitive, the error is not during
> instantiation of the closure object's type, it's during instantiation
> of the function template. The type of the closure will always be the
> type of the closure, but the type of the explicit object parameter can
> be different. In the following example we don't go into
> tsubst_lambda_expr at all.

Ah, good point.

> I hope this demonstrates that placing the check in tsubst_function_decl
> is the correct way to do this.

Sounds right.

> Now with that out of the way, I do still kind of feel icky about it.
> This really is a MASSIVE edge case that will almost never matter. As
> far as I know, instantiating explicitly like so...
> 
> auto f = [x = 42](this auto&&) -> int { return x; };
> int (*fp)(int&) = &decltype(f)::operator();
> 
> ...is the only way to coerce the explicit object parameter of the
> lambda's call operator into deducing as an unrelated type. Cases that
> are not deduced can be caught trivially while parsing the lambda and
> are the only reasonable cases where you might have an unrelated type.
> Perhaps it might become relevant in the future if a proposal like
> https://atomgalaxy.github.io/isocpp-1107/D1107.html ever made it in,
> but we aren't there yet. So as it stands, I'm pretty certain that it's
> just impossible to instantiate a lambda's call operator with an
> unrelated xobj parameter type except for the above case with
> address-of. If you can think of another, please do share, but I've
> spent a fair amount of time on it and came up with nothing.

I think you're right.

> Anyway, due to this, I am not currently concerned about the lack of a
> diagnostic, and I believe my implementation is the most correct way to
> do it. The problem with resolve_address_of_overloaded_function that I
> go into below (depending on if you agree that it's a problem) is what
> needs to be fixed to allow the current diagnostic to be properly
> emitted. As of right now, there is no execution path that I know of
> where the diagnostic will be properly emitted.
> 
> I go into more detail about this in the comment in
> tsubst_function_decl, although I do have to revise it a bit as I wrote
> it before I had a discussion with another developer on the correct
> behavior of the following case. I previously wondered if the overload
> resolution from initializing p1 should result in a hard fault.
> 
> template<typename T>
> struct S : T {
>    using T::operator();
>    void operator()(this int&, auto) {}
> };
> 
> int main()
> {
>    auto s0 = S{[](this auto&&, auto){}};
>    // error, ambiguous
>    void (*p0)(int&, int) = &decltype(s0)::operator();
> 
>    auto s1 = S{[x = 42](this auto&&, auto){}};
>    // SFINAE's, calls S::operator()
>    void (*p1)(int&, int) = &decltype(s1)::operator();
> }
> 
> The wording in [expr.prim.lambda.closure-5] is similar to that in
> [dcl.fct-15], and we harness SFINAE in that case, so he concluded that
> this case (initializing p1) should also harness SFINAE.

Agreed.

>>> The other problem I'm having is
>>>
>>> auto f0 = [n = 5, &m](this auto const&){ n = 10; };
>>> This errors just fine, the lambda is unconditionally const so
>>> LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
>>>
>>> This on the other hand does not. The constness of the captures depends
>>> on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
>>> here.
>>> auto f1 = [n = 5](this auto&& self){ n = 10; };
>>> as_const(f1)();
>>
>> That sounds correct, why would this be an error?
>>
>> The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P,
>> it depends on the type of the object parameter, which in this case is
>> non-const, so so are the captures.
> 
> Oops, I should have just made a less terse example, you missed the
> "as_const" in the call to the lambda. The object parameter should be
> deduced as const here. I definitely should have made that example
> better, my bad.

Ah, yes, I see it now.

>>> I was going to close out this message there but I just realized why
>>> exactly you think erroring in instantiate_body is too late, it's
>>> because at that point an error is an error, while if we error a little
>>> bit earlier during substitution it's not a hard error. I'm glad I
>>> realized this now, because I am much more confident in how to implement
>>> the errors for unrelated type now. I'm still a little confused on what
>>> the exact protocol for emitting errors at this stage is, as there
>>> aren't many explicit errors written in. It seems to me like the errors
>>> are supposed to be emitted when calling back into non-template stages
>>> of the compiler with substituted types. It also seems like that hasn't
>>> always been strictly followed, and I hate to contribute to code debt
>>> but I'm not sure how I would do it in this case, nor if I actually have
>>> a valid understanding of how this all works.
>>
>> Most errors that could occur in template substitution are guarded by if
>> (complain & tf_error) so that they aren't emitted during deduction
>> substitution. It's better if those are in code that's shared with the
>> non-template case, but sometimes that isn't feasible.
> 
> Yeah, makes sense, but the case going through
> resolve_address_of_overloaded_function -> fn_type_unification ->
> instantiate_template -> tsubst_decl -> tsubst_function_decl does not
> behave super nicely. I tried adding (complain & tf_error) to the
> explain_p parameter of fn_type_unification in
> resolve_address_of_overloaded_function, but I immediately learned why
> that wasn't being handled that way. I think
> resolve_address_of_overloaded_function has a number of problems and
> should probably use print_z_candidates instead of print_candidates in
> the long term. This would actually solve the problem in
> tsubst_function_decl where it never emits an error for an unrelated
> type, print_z_candidates does call fn_type_unification with true passed
> to explain_p. I go into this in detail in the large comment in
> tsubst_function_decl (that I have to revise) so I won't waste time
> rehashing it here.

Makes sense, we won't see a message about the unrelated type because 
print_candidates doesn't repeat deduction like print_z_candidates does.

> Thank you for explaining though, some of the code doesn't reflect the
> method you've relayed here (such as an unguarded error_at in
> tsubst_function_decl) so I was starting to get confused on what exactly
> is supposed to be going on. This clarifies things for me.

That unguarded error does indeed look suspicious, though it could be 
that OMP reductions are called in a sufficiently unusual way that it 
isn't actually a problem in practice. /shrug

Jason

> > Now with that out of the way, I do still kind of feel icky about it.
> > This really is a MASSIVE edge case that will almost never matter. As
> > far as I know, instantiating explicitly like so...
> > 
> > auto f = [x = 42](this auto&&) -> int { return x; };
> > int (*fp)(int&) = &decltype(f)::operator();
> > 
> > ...is the only way to coerce the explicit object parameter of the
> > lambda's call operator into deducing as an unrelated type. Cases that
> > are not deduced can be caught trivially while parsing the lambda and
> > are the only reasonable cases where you might have an unrelated type.
> > Perhaps it might become relevant in the future if a proposal like
> > https://atomgalaxy.github.io/isocpp-1107/D1107.html ever made it in,
> > but we aren't there yet. So as it stands, I'm pretty certain that it's
> > just impossible to instantiate a lambda's call operator with an
> > unrelated xobj parameter type except for the above case with
> > address-of. If you can think of another, please do share, but I've
> > spent a fair amount of time on it and came up with nothing.
> 
> 
> I think you're right.
> 
I was about to send a quick e-mail amending this, before I respond to
everything else I want to include this test case I just came up with minutes
ago.

template<typename T>
struct S : T {
  using T::operator();
  operator int() const {return {};}
};

int main()
{
  auto s0 = S{[](this auto&& self) { return self; }};
  auto s1 = S{[x = 0](this auto&& self) { return self; }};

  s0.operator()<int>();
  s1.operator()<int>();
}

So I was wrong, but, the good news is that this does demonstrate that there
is a code path where my diagnostic works.

template<typename T>
concept NotInt = (!__is_same (T, int));

template<bool> struct enable_if {};
template<> struct enable_if<true> { using type = decltype(nullptr); };
template<bool B> using enable_if_t = typename enable_if<B>::type;

template<NotInt T>
void using_concepts(T) {}

template<typename T, enable_if_t<!__is_same (T, int)> = nullptr>
void using_enable_if(T) {}

void test()
{
  void (*fp_concepts)(int) = &using_concepts;
  void (*fp_enable_if)(int) = &using_enable_if;

  using_concepts(0);
  using_enable_if(0);
}

I also have this test case that demonstrates the difference in diagnostic
quality. This is unrelated to explicit object member functions though, but
it does demonstrate that the diagnostics that I currently produce are in
equal quality to the ones already produced in these cases.

At this point I feel like I am unlikely to start fixing the bug with
captures not being treated as const tonight. Cleaning up the tests is taking
me longer than I thought.

Anyway I'm just rushing this e-mail to clarify this mistake, admittedly I am
a little excited to have found (which in hindsight should have been obvious)
a test case that more directly calls a lambda's call operator with an
unrelated type.

Alex

> > > > OKAY, I figured out SOMETHING, I think this should be fine. As noted in
> > > > the comments, this might be a better way of handling the static lambda
> > > > case too. This is still a nasty hack so it should probably be done
> > > > differently, but I question if making a whole new fntype node in
> > > > tsubst_lambda_expr makes sense. On the other hand, maybe there will be
> > > > problems if a lambda is already instantiated? I'm not sure, mutating
> > > > things this way makes me uneasy though.
> > > 
> > > A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is that
> > > all equivalent FUNCTION_TYPE share the same tree node (through
> > > type_hash_canon), so you're messing with the type of unrelated functions
> > > at the same time. I think it's better to stick with the way static
> > > lambdas are handled.
> > 
> > Yes, that was my concern as well, without even thinking about hashing.
> > I will investigate it deeper but I'm pretty sure TYPE_METHOD_BASETYPE,
> > while a valid field, is not used at all for function_type nodes. I had
> > to look into this when looking into the DECL_CONTEXT stuff previously,
> > so I'm pretty confident in this. Come to think of it, does hashing even
> > take fields that aren't "supposed" to be set for a node into account?
> 
> 
> It doesn't. The issue is messing with the type of (potentially) a lot
> of different functions. Even if it doesn't actually break anything, it
> seems like the kind of hidden mutation that you were objecting to.

Oh... yeah..., I see the issue now. I still don't think the solution
used for static lambdas will work, or be painless anyhow, but if I
can't find something better I will try to use that one.

> > Well, even so, I can just clear it after it gets used as we just need
> > it to pass the closure type down. Perhaps I should have led with this,
> > but as it stands the version that uses TYPE_METHOD_BASETYPE bootstraps
> > with no regressions. I'll still look deeper but I'm pretty confident in
> > my decision here, I really don't want to try to unravel what
> > build_memfn_type does, I would rather find a whole different way of
> > passing that information down.
> 
> 
> But the existing code already works fine, it's just a question of
> changing the conditions so we handle xob lambdas the same way as static.

I'm still concerned it wont cooperate with xobj parameters of unrelated
type, but like I said, you've indicated my solution is definitely wrong
so I'll look at fixing it.

> > Anyway, due to this, I am not currently concerned about the lack of a
> > diagnostic, and I believe my implementation is the most correct way to
> > do it. The problem with resolve_address_of_overloaded_function that I
> > go into below (depending on if you agree that it's a problem) is what
> > needs to be fixed to allow the current diagnostic to be properly
> > emitted. As of right now, there is no execution path that I know of
> > where the diagnostic will be properly emitted.
> > 
> > I go into more detail about this in the comment in
> > tsubst_function_decl, although I do have to revise it a bit as I wrote
> > it before I had a discussion with another developer on the correct
> > behavior of the following case. I previously wondered if the overload
> > resolution from initializing p1 should result in a hard fault.
> > 
> > template<typename T>
> > struct S : T {
> > using T::operator();
> > void operator()(this int&, auto) {}
> > };
> > 
> > int main()
> > {
> > auto s0 = S{[](this auto&&, auto){}};
> > // error, ambiguous
> > void (*p0)(int&, int) = &decltype(s0)::operator();
> > 
> > auto s1 = S{[x = 42](this auto&&, auto){}};
> > // SFINAE's, calls S::operator()
> > void (*p1)(int&, int) = &decltype(s1)::operator();
> > }
> > 
> > The wording in [expr.prim.lambda.closure-5] is similar to that in
> > [dcl.fct-15], and we harness SFINAE in that case, so he concluded that
> > this case (initializing p1) should also harness SFINAE.
> 
> 
> Agreed.

Perfect, glad we are on the same page there.

> > > > The other problem I'm having is
> > > > 
> > > > auto f0 = [n = 5, &m](this auto const&){ n = 10; };
> > > > This errors just fine, the lambda is unconditionally const so
> > > > LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
> > > > 
> > > > This on the other hand does not. The constness of the captures depends
> > > > on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
> > > > here.
> > > > auto f1 = [n = 5](this auto&& self){ n = 10; };
> > > > as_const(f1)();
> > > 
> > > That sounds correct, why would this be an error?
> > > 
> > > The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P,
> > > it depends on the type of the object parameter, which in this case is
> > > non-const, so so are the captures.
> > 
> > Oops, I should have just made a less terse example, you missed the
> > "as_const" in the call to the lambda. The object parameter should be
> > deduced as const here. I definitely should have made that example
> > better, my bad.
> 
> 
> Ah, yes, I see it now.

I don't remember if I relayed my planned fix for this to you. My
current idea is to modify the tree during instantiation of the lambda's
body somewhere near tsubst and apply const to all it's members. This is
unfortunately the best idea I have so far and it feels like an awful
hack. I am open to better ideas, but I don't think we can do anything
until the template is instantiated so I think it has to be there.

> > > > I was going to close out this message there but I just realized why
> > > > exactly you think erroring in instantiate_body is too late, it's
> > > > because at that point an error is an error, while if we error a little
> > > > bit earlier during substitution it's not a hard error. I'm glad I
> > > > realized this now, because I am much more confident in how to implement
> > > > the errors for unrelated type now. I'm still a little confused on what
> > > > the exact protocol for emitting errors at this stage is, as there
> > > > aren't many explicit errors written in. It seems to me like the errors
> > > > are supposed to be emitted when calling back into non-template stages
> > > > of the compiler with substituted types. It also seems like that hasn't
> > > > always been strictly followed, and I hate to contribute to code debt
> > > > but I'm not sure how I would do it in this case, nor if I actually have
> > > > a valid understanding of how this all works.
> > > 
> > > Most errors that could occur in template substitution are guarded by if
> > > (complain & tf_error) so that they aren't emitted during deduction
> > > substitution. It's better if those are in code that's shared with the
> > > non-template case, but sometimes that isn't feasible.
> > 
> > Yeah, makes sense, but the case going through
> > resolve_address_of_overloaded_function -> fn_type_unification ->
> > instantiate_template -> tsubst_decl -> tsubst_function_decl does not
> > behave super nicely. I tried adding (complain & tf_error) to the
> > explain_p parameter of fn_type_unification in
> > resolve_address_of_overloaded_function, but I immediately learned why
> > that wasn't being handled that way. I think
> > resolve_address_of_overloaded_function has a number of problems and
> > should probably use print_z_candidates instead of print_candidates in
> > the long term. This would actually solve the problem in
> > tsubst_function_decl where it never emits an error for an unrelated
> > type, print_z_candidates does call fn_type_unification with true passed
> > to explain_p. I go into this in detail in the large comment in
> > tsubst_function_decl (that I have to revise) so I won't waste time
> > rehashing it here.
> 
> 
> Makes sense, we won't see a message about the unrelated type because
> print_candidates doesn't repeat deduction like print_z_candidates does.
> 
> > Thank you for explaining though, some of the code doesn't reflect the
> > method you've relayed here (such as an unguarded error_at in
> > tsubst_function_decl) so I was starting to get confused on what exactly
> > is supposed to be going on. This clarifies things for me.
> 
> 
> That unguarded error does indeed look suspicious, though it could be
> that OMP reductions are called in a sufficiently unusual way that it
> isn't actually a problem in practice. /shrug
> 
> Jason

Fair enough.

Luckily, since I found that other test case, I have been able to
confirm that my diagnostic is properly reported when print_z_candidates
is called. So I feel like the diagnostic is implemented correctly.

Should I wait until I fix the issue in tsubst_lambda_expr before
submitting the patch? I'm fine to do it either way, just whatever you
prefer. If I finish cleaning up these tests before I hear back I'll go
ahead and submit it and then start looking at different solutions in
there.

Alex

On 11/26/23 20:44, waffl3x wrote:
>>>>> The other problem I'm having is
>>>>>
>>>>> auto f0 = [n = 5, &m](this auto const&){ n = 10; };
>>>>> This errors just fine, the lambda is unconditionally const so
>>>>> LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
>>>>>
>>>>> This on the other hand does not. The constness of the captures depends
>>>>> on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
>>>>> here.
>>>>> auto f1 = [n = 5](this auto&& self){ n = 10; };
>>>>> as_const(f1)();
>>>>
>>>> That sounds correct, why would this be an error?
>>>>
>>>> The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P,
>>>> it depends on the type of the object parameter, which in this case is
>>>> non-const, so so are the captures.
>>>
>>> Oops, I should have just made a less terse example, you missed the
>>> "as_const" in the call to the lambda. The object parameter should be
>>> deduced as const here. I definitely should have made that example
>>> better, my bad.
>>
>> Ah, yes, I see it now.
> 
> I don't remember if I relayed my planned fix for this to you. My
> current idea is to modify the tree during instantiation of the lambda's
> body somewhere near tsubst and apply const to all it's members. This is
> unfortunately the best idea I have so far and it feels like an awful
> hack. I am open to better ideas, but I don't think we can do anything
> until the template is instantiated so I think it has to be there.

I think the answer should be in lambda_proxy_type.  The case where we 
build a DECLTYPE_TYPE may need to be expanded to cover this situation.

> Should I wait until I fix the issue in tsubst_lambda_expr before
> submitting the patch? I'm fine to do it either way, just whatever you
> prefer. If I finish cleaning up these tests before I hear back I'll go
> ahead and submit it and then start looking at different solutions in
> there.

Go ahead and submit.

Jason

I think this is cleaned up enough to be presentable. Bootstrapped but
not tested but I don't think I changed anything substantial. I am
running tests right now and will report if anything fails. I have not
fixed the problem in tsubst_lambda_expr that we talked about, that will
be first on my list tomorrow. While writing/cleaning up tests I had to
investigate some things, one of which is calling an overloaded
function, where one of the candidates are introduced by a using
declaration, is considered ambiguous. I haven't narrowed down the case
for this yet so I don't know if it's related to xobj member
functions/lambda with xobj parameters or not. I had to pull a few tests
because of it though.

I did not get as much done as I would have hoped today. This really
just serves as a small progress update. Once again, todo is the issue
you raised in tsubst_lambda_expr, and fixing handling of captures when
a const xobj parameter is deduced in a lamdba call operator.

Alex

On Sunday, November 26th, 2023 at 7:40 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/26/23 20:44, waffl3x wrote:
> 
> > > > > > The other problem I'm having is
> > > > > > 
> > > > > > auto f0 = [n = 5, &m](this auto const&){ n = 10; };
> > > > > > This errors just fine, the lambda is unconditionally const so
> > > > > > LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
> > > > > > 
> > > > > > This on the other hand does not. The constness of the captures depends
> > > > > > on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
> > > > > > here.
> > > > > > auto f1 = [n = 5](this auto&& self){ n = 10; };
> > > > > > as_const(f1)();
> > > > > 
> > > > > That sounds correct, why would this be an error?
> > > > > 
> > > > > The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P,
> > > > > it depends on the type of the object parameter, which in this case is
> > > > > non-const, so so are the captures.
> > > > 
> > > > Oops, I should have just made a less terse example, you missed the
> > > > "as_const" in the call to the lambda. The object parameter should be
> > > > deduced as const here. I definitely should have made that example
> > > > better, my bad.
> > > 
> > > Ah, yes, I see it now.
> > 
> > I don't remember if I relayed my planned fix for this to you. My
> > current idea is to modify the tree during instantiation of the lambda's
> > body somewhere near tsubst and apply const to all it's members. This is
> > unfortunately the best idea I have so far and it feels like an awful
> > hack. I am open to better ideas, but I don't think we can do anything
> > until the template is instantiated so I think it has to be there.
> 
> 
> I think the answer should be in lambda_proxy_type. The case where we
> build a DECLTYPE_TYPE may need to be expanded to cover this situation.
> 
> > Should I wait until I fix the issue in tsubst_lambda_expr before
> > submitting the patch? I'm fine to do it either way, just whatever you
> > prefer. If I finish cleaning up these tests before I hear back I'll go
> > ahead and submit it and then start looking at different solutions in
> > there.
> 
> 
> Go ahead and submit.
> 
> Jason

I'm going to need to sit down and set up a proper e-mail application
once I'm done with all this, I missed your reply because it went off to
another thread. Luckily, I decided to send the patch anyway, and when I
noticed that my patch was not under the same thread I came looking for
it. Ah well, what a pain, I guess getting used to these mail list
things is just going to take time.

> > It doesn't. The issue is messing with the type of (potentially) a lot
> > of different functions. Even if it doesn't actually break anything, it
> > seems like the kind of hidden mutation that you were objecting to.
> 
> 
> Oh... yeah..., I see the issue now. I still don't think the solution
> used for static lambdas will work, or be painless anyhow, but if I
> can't find something better I will try to use that one.
> 
> > > Well, even so, I can just clear it after it gets used as we just need
> > > it to pass the closure type down. Perhaps I should have led with this,
> > > but as it stands the version that uses TYPE_METHOD_BASETYPE bootstraps
> > > with no regressions. I'll still look deeper but I'm pretty confident in
> > > my decision here, I really don't want to try to unravel what
> > > build_memfn_type does, I would rather find a whole different way of
> > > passing that information down.
> > 
> > But the existing code already works fine, it's just a question of
> > changing the conditions so we handle xob lambdas the same way as static.
> 
> 
> I'm still concerned it wont cooperate with xobj parameters of unrelated
> type, but like I said, you've indicated my solution is definitely wrong
> so I'll look at fixing it.

I spent some time looking at it, I've decided you're probably right
that handling this the same way as the static lambda case is the best
in the short term. I still don't like it, but I've gone ahead and made
that change, and it seems to work just fine. I still find it icky, but
once I realized we do in fact need lambda_fntype since it might have
been substituted into in tsubst_lambda_expr, I don't see any better way
of doing this at the moment.

Since the added parameter just gets popped off by static_fn_type, and
tsubst_lambda_expr doesn't touch the xobj parameter, I'm pretty sure it
should behave properly. So no problems I guess, moving on to the
captures bug.

Alex

This fixes the const bug. I haven't bootstrapped and tested beyond my
own tests yet but this does it. I don't know if this is the right way
to fix this yet, but I think it's pretty close. I'll see if I can make
a better write up tomorrow, but it seems to me that since we never
cared about substituting captures at function template instantiation
time there was never a special case added in here to add the const over
to the var_decl's type. I assume in cases where there is some kind of
type mismatch that it's usually handled before here, but this special
case throws a wrench into the works.

@@ -15402,6 +15450,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain,
                  gcc_checking_assert (TYPE_MAIN_VARIANT (TREE_TYPE (ve))
                                       == TYPE_MAIN_VARIANT (type));
                SET_DECL_VALUE_EXPR (r, ve);
+               if (is_capture_proxy (t))
+                 type = TREE_TYPE (ve);
              }
            if (CP_DECL_THREAD_LOCAL_P (r)
                && !processing_template_decl)

Sigh, that was really tough to track down, but it's honestly pretty
relieving that it was MOSTLY using the closure type and not totally
ignoring it. It just misses a step here for some reason, idk if this is
a larger bug that was benign before, or if this case just invalidates
some assumptions, I'm not really sure. I don't really have the energy
to fully understand everything that's going on in here tonight. This
fix is just good enough to bootstrap and run the tests overnight and
hopefully when I check in the morning there won't be any regressions.

Going to get some rest now, as long as the other maybe bugs I found are
not related to xobj parameters we can probably dust off this patch this
week. Although, maybe I should have learned by now that I'm terrible at
estimating timelines.

Alex

On 11/27/23 00:35, waffl3x wrote:
> I think this is cleaned up enough to be presentable. Bootstrapped but
> not tested but I don't think I changed anything substantial. I am
> running tests right now and will report if anything fails. I have not
> fixed the problem in tsubst_lambda_expr that we talked about, that will
> be first on my list tomorrow. While writing/cleaning up tests I had to
> investigate some things, one of which is calling an overloaded
> function, where one of the candidates are introduced by a using
> declaration, is considered ambiguous. I haven't narrowed down the case
> for this yet so I don't know if it's related to xobj member
> functions/lambda with xobj parameters or not. I had to pull a few tests
> because of it though.
> 
> I did not get as much done as I would have hoped today. This really
> just serves as a small progress update. Once again, todo is the issue
> you raised in tsubst_lambda_expr, and fixing handling of captures when
> a const xobj parameter is deduced in a lamdba call operator.

> +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> +#define DECL_XOBJ_MEMBER_FUNC_P(NODE)                  \
> +#define DECL_OBJECT_MEMBER_FUNC_P(NODE) \

Let's use the full word FUNCTION in these macros for consistency with 
DECL_STATIC_FUNCTION_P.

> @@ -6544,7 +6544,7 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
>        tree fn_first_arg = NULL_TREE;
>        const vec<tree, va_gc> *fn_args = args;
>  
> -      if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn))
> +      if (DECL_OBJECT_MEMBER_FUNC_P (fn))
>         {
>           /* Figure out where the object arg comes from.  If this
>              function is a non-static member and we didn't get an

Hmm, that this function explicitly pulls out the object argument into 
first_arg strengthens your earlier argument that we shouldn't bother 
trying to handle null first_arg.  But let's not mess with that in this 
patch.

> +      val = handle_arg(TREE_VALUE (parm),

Missing space.

>           /* We know an iobj parameter must be a reference. If our xobj                                                             
>              parameter is a pointer, we know this is not a redeclaration.                                                           
>              This also catches array parameters, those are pointers too.  */
>           if (TYPE_PTR_P (xobj_param))
>             continue;

Handling pointers specifically here seems unnecessary, they should be 
rare and will be handled by the next check for unrelated type.

>              dealing with a by-value xobj parameter we can procede following                                                        

"proceed"

>    /* An operator function must either be a non-static member function
>       or have at least one parameter of a class, a reference to a class,
>       an enumeration, or a reference to an enumeration.  13.4.0.6 */
> -  if (! methodp || DECL_STATIC_FUNCTION_P (decl))
> +  /* This can just be DECL_STATIC_FUNCTION_P (decl) I think?  */
> +  if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
> +      || DECL_STATIC_FUNCTION_P (decl))

No, it also needs to be true for non-members; rather, I think it can 
just be if (!DECL_OBJECT_FUNCTION_P (decl))

> +  if (xobj_param)
> +    {
> +      quals = TYPE_UNQUALIFIED;
> +      if (TYPE_REF_P (xobj_param)
> +         && !(cp_type_quals (TREE_TYPE (xobj_param)) & TYPE_QUAL_CONST))
> +        LAMBDA_EXPR_MUTABLE_P (lambda_expr) = 1;
> +    }

I don't think we want to mess with MUTABLE_P here.  But then 
capture_decltype needs to be fixed to refer to the type of the object 
parameter rather than MUTABLE_P.

Actually, I think we can do away with the MUTABLE_P flag entirely.  I'm 
going to push a patch to do that.

> -       if (!LAMBDA_EXPR_STATIC_P (lambda_expr))
> +       if (!LAMBDA_EXPR_STATIC_P (lambda_expr)
> +           && !DECL_XOBJ_MEMBER_FUNC_P (fco))

This could be if (DECL_IOBJ_MEMBER_FUNCTION_P (fco))

> -  if (closure && !DECL_STATIC_FUNCTION_P (t))
> +  if (closure && DECL_IOBJ_MEMBER_FUNC_P (t) && !DECL_STATIC_FUNCTION_P (t))

This shouldn't need to still check DECL_STATIC_FUNCTION_P.

> +  /* We don't touch a lambda's func when it's just trying to create the
> +     closure type.  */

We need to check it somewhere, currently this crashes:

template <class T> void f()
{
   int i;
   [=](this T&& self){ return i; }(); // error, unrelated
}
int main() { f<int>(); }

> @@ -3691,18 +3691,7 @@ build_min_non_dep_op_overload (enum tree_code op,
>    releasing_vec args;
>    va_start (p, overload);
>  
> -  if (TREE_CODE (TREE_TYPE (overload)) == FUNCTION_TYPE)
> -    {
> -      fn = overload;
> -      if (op == ARRAY_REF)
> -       obj = va_arg (p, tree);
> -      for (int i = 0; i < nargs; i++)
> -       {
> -         tree arg = va_arg (p, tree);
> -         vec_safe_push (args, arg);
> -       }
> -    }

Maybe change the test to !DECL_OBJECT_MEMBER_FUNC_P to avoid reordering 
the cases?

> @@ -15402,6 +15450,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain,
>                   gcc_checking_assert (TYPE_MAIN_VARIANT (TREE_TYPE (ve))
>                                        == TYPE_MAIN_VARIANT (type));
>                 SET_DECL_VALUE_EXPR (r, ve);
> +               if (is_capture_proxy (t))
> +                 type = TREE_TYPE (ve);

That should have close to the same effect as the lambda_proxy_type 
adjustment I was talking about, since that function basically returns 
the TREE_TYPE of the COMPONENT_REF.  But the underlying problem is that 
finish_non_static_data_member assumes that 'object' is '*this', for 
which you can trust the cv-quals; for auto&&, you can't. 
capture_decltype has the same problem.  I'm attaching a patch to address 
this in both places.

Jason

On Wednesday, November 29th, 2023 at 10:00 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/27/23 00:35, waffl3x wrote:
> 
> > I think this is cleaned up enough to be presentable. Bootstrapped but
> > not tested but I don't think I changed anything substantial. I am
> > running tests right now and will report if anything fails. I have not
> > fixed the problem in tsubst_lambda_expr that we talked about, that will
> > be first on my list tomorrow. While writing/cleaning up tests I had to
> > investigate some things, one of which is calling an overloaded
> > function, where one of the candidates are introduced by a using
> > declaration, is considered ambiguous. I haven't narrowed down the case
> > for this yet so I don't know if it's related to xobj member
> > functions/lambda with xobj parameters or not. I had to pull a few tests
> > because of it though.
> > 
> > I did not get as much done as I would have hoped today. This really
> > just serves as a small progress update. Once again, todo is the issue
> > you raised in tsubst_lambda_expr, and fixing handling of captures when
> > a const xobj parameter is deduced in a lamdba call operator.
> 
> > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> > +#define DECL_XOBJ_MEMBER_FUNC_P(NODE) \
> > +#define DECL_OBJECT_MEMBER_FUNC_P(NODE) \
> 
> 
> Let's use the full word FUNCTION in these macros for consistency with
> DECL_STATIC_FUNCTION_P.

Okay.

> > @@ -6544,7 +6544,7 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
> > tree fn_first_arg = NULL_TREE;
> > const vec<tree, va_gc> *fn_args = args;
> > 
> > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn))
> > + if (DECL_OBJECT_MEMBER_FUNC_P (fn))
> > {
> > /* Figure out where the object arg comes from. If this
> > function is a non-static member and we didn't get an
> 
> 
> Hmm, that this function explicitly pulls out the object argument into
> first_arg strengthens your earlier argument that we shouldn't bother
> trying to handle null first_arg. But let's not mess with that in this
> patch.

Maybe I'll take some time to look into it when this patch is done, I
came across another block that seems to guarantee that first_arg gets
passed in a bit ago.

> > + val = handle_arg(TREE_VALUE (parm),
> 
> 
> Missing space.

Is there a script I can use for this so I'm not wasting your time on
little typos like this one?

> > /* We know an iobj parameter must be a reference. If our xobj
> > parameter is a pointer, we know this is not a redeclaration.
> > This also catches array parameters, those are pointers too. */
> > if (TYPE_PTR_P (xobj_param))
> > continue;
> 
> 
> Handling pointers specifically here seems unnecessary, they should be
> rare and will be handled by the next check for unrelated type.

Ah, it took me a second but I see it now, yeah I think I'll make this
change, with a comment that notes that it also handles the pointer case.

> > dealing with a by-value xobj parameter we can procede following
> 
> 
> "proceed"
> 
> > /* An operator function must either be a non-static member function
> > or have at least one parameter of a class, a reference to a class,
> > an enumeration, or a reference to an enumeration. 13.4.0.6 /
> > - if (! methodp || DECL_STATIC_FUNCTION_P (decl))
> > + / This can just be DECL_STATIC_FUNCTION_P (decl) I think? */
> > + if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
> > + || DECL_STATIC_FUNCTION_P (decl))
> 
> 
> No, it also needs to be true for non-members; rather, I think it can
> just be if (!DECL_OBJECT_FUNCTION_P (decl))

Yeah that seems to make sense, I'll try that.

> > + if (xobj_param)
> > + {
> > + quals = TYPE_UNQUALIFIED;
> > + if (TYPE_REF_P (xobj_param)
> > + && !(cp_type_quals (TREE_TYPE (xobj_param)) & TYPE_QUAL_CONST))
> > + LAMBDA_EXPR_MUTABLE_P (lambda_expr) = 1;
> > + }
> 
> 
> I don't think we want to mess with MUTABLE_P here. But then
> capture_decltype needs to be fixed to refer to the type of the object
> parameter rather than MUTABLE_P.
> 
> Actually, I think we can do away with the MUTABLE_P flag entirely. I'm
> going to push a patch to do that.

I've been working on code that concerns it today and yesterday and I
was beginning to think the same thing. My current version doesn't set
it at all. I'm happy to see it removed.

When I looked into whether I should or should not be setting it I found
a new bug with decltype((capture)) in lambdas with default capture. I
am not going to try to fix the value capture default case but I was
able to fix the ref capture default case. Basically decltype((x)) is
not dependent right now as far as I can tell. Both MSVC and clang have
the same bug so I'm not worried about it tbh.

> > - if (!LAMBDA_EXPR_STATIC_P (lambda_expr))
> > + if (!LAMBDA_EXPR_STATIC_P (lambda_expr)
> > + && !DECL_XOBJ_MEMBER_FUNC_P (fco))
> 
> 
> This could be if (DECL_IOBJ_MEMBER_FUNCTION_P (fco))
> 
> > - if (closure && !DECL_STATIC_FUNCTION_P (t))
> > + if (closure && DECL_IOBJ_MEMBER_FUNC_P (t) && !DECL_STATIC_FUNCTION_P (t))
> 
> 
> This shouldn't need to still check DECL_STATIC_FUNCTION_P.

Yeah... not sure why I left that.

> > + /* We don't touch a lambda's func when it's just trying to create the
> > + closure type. */
> 
> 
> We need to check it somewhere, currently this crashes:
> 
> template <class T> void f()
> 
> {
> int i;
> [=](this T&& self){ return i; }(); // error, unrelated
> }
> int main() { f<int>(); }

Ah, yep you're right, how silly of me to forget that case. I'll make
sure to add it as a test case as well.

> > @@ -3691,18 +3691,7 @@ build_min_non_dep_op_overload (enum tree_code op,
> > releasing_vec args;
> > va_start (p, overload);
> > 
> > - if (TREE_CODE (TREE_TYPE (overload)) == FUNCTION_TYPE)
> > - {
> > - fn = overload;
> > - if (op == ARRAY_REF)
> > - obj = va_arg (p, tree);
> > - for (int i = 0; i < nargs; i++)
> > - {
> > - tree arg = va_arg (p, tree);
> > - vec_safe_push (args, arg);
> > - }
> > - }
> 
> 
> Maybe change the test to !DECL_OBJECT_MEMBER_FUNC_P to avoid reordering
> the cases?

I thought you had already given the thumbs up to this, but I'll take
another look at it and see what feels good.

> > @@ -15402,6 +15450,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain,
> > gcc_checking_assert (TYPE_MAIN_VARIANT (TREE_TYPE (ve))
> > == TYPE_MAIN_VARIANT (type));
> > SET_DECL_VALUE_EXPR (r, ve);
> > + if (is_capture_proxy (t))
> > + type = TREE_TYPE (ve);
> 
> 
> That should have close to the same effect as the lambda_proxy_type
> adjustment I was talking about, since that function basically returns
> the TREE_TYPE of the COMPONENT_REF. But the underlying problem is that
> finish_non_static_data_member assumes that 'object' is '*this', for
> which you can trust the cv-quals; for auto&&, you can't.
> capture_decltype has the same problem. I'm attaching a patch to address
> this in both places.
> 
> Jason

Ah perfect, you also noticed the decltype problem. I don't think I
could have fixed this one on my own so thanks for addressing it.

I started replacing old uses of DECL_NONSTATIC_MEMBER_FUNCTION_P in the
cases where I could tell for sure what the correct thing to do is and I
ended up finding a few bugs.

template<typename T>
concept is_int = __is_same(T, int);

struct S {
  static int f(is_int auto a) { return 5; };
  int f(this S&, auto a) { return 10; };
};

int main()
{
  S s{};
  return s.f(0);
}

This program returns 10 instead of 5.

https://godbolt.org/z/jMfbbaczr
Actually, I found that this bug exists in MSVC as well. Here it is in
godbolt with some extra things to make it more obvious where the bug
is. I've already found the cause of it, cand_parms_match doesn't ignore
the xobj parameter. It's an easy fix thankfully. I feel like these can
be unified because I'm pretty sure I've seen this in 2 or 3 places, but
I'll concern myself with trying to refactor it later.

I still have a case that I haven't fully narrowed down yet that
considers a call ambiguous when one of the candidates were introduce by
a using declaration. That's next on my list I believe.

I also found that xobj conversion operators were not working. I have
already fixed it but I need to write more robust tests for it to make
sure all the cases work. There's also a case where an xobj conversion
operator has an unrelated type for it's parameter and I haven't been
able to decide if it's ill-formed or not. I believe it would break
conversion sequence rules for a conversion operator to be selected.
Once I finish everything else I'll get a good robust test case for it.

This ended up longer than I thought. To recap, I have the bug with
constraints which should be fixed but needs tests, I have the wrongly
ambiguous overload bug that I need to narrow down and find, and I have
xobj conversion operators to write tests for. So after I address what
you said that's whats on my list. There's also integrating your patch
which I'm not totally sure how I'm supposed to do but I think I have a
good idea of it so I should be fine.

I feel like I'm forgetting something, but that often happens when I
stack up like 4 different things on my mind at once so it's probably
nothing. I'll get back to work. At this rate it almost feels like we
will be able to get this finished by the end of the week.

Alex

On 11/30/23 01:36, waffl3x wrote:
> On Wednesday, November 29th, 2023 at 10:00 PM, Jason Merrill <jason@redhat.com> wrote:
>> On 11/27/23 00:35, waffl3x wrote:
>>
>>> + val = handle_arg(TREE_VALUE (parm),
>>
>> Missing space.
> 
> Is there a script I can use for this so I'm not wasting your time on
> little typos like this one?

contrib/check_GNU_style.py seems like what you're looking for.

> This ended up longer than I thought. To recap, I have the bug with
> constraints which should be fixed but needs tests, I have the wrongly
> ambiguous overload bug that I need to narrow down and find, and I have
> xobj conversion operators to write tests for. So after I address what
> you said that's whats on my list. There's also integrating your patch
> which I'm not totally sure how I'm supposed to do but I think I have a
> good idea of it so I should be fine.

I think I'll push that patch as well so you can just rebase.

> I feel like I'm forgetting something, but that often happens when I
> stack up like 4 different things on my mind at once so it's probably
> nothing. I'll get back to work. At this rate it almost feels like we
> will be able to get this finished by the end of the week.

Sounds good.

Jason

I ran into another issue while devising tests for redeclarations of
xobj member functions as static member functions and vice versa. I am
pretty sure by the literal wording of the standard, this is well formed.

template<typename T>
concept Constrain = true;

struct S {
  void f(this auto, Constrain auto) {};
  static void f(Constrain auto) {};

  void g(this auto const&, Constrain auto) {};
  static void g(Constrain auto) {};

  void h(this auto&&, Constrain auto) {};
  static void h(Constrain auto) {};
};

And also,

struct S{
  void f(this auto) {};
  static void f() {};

  void g(this auto const&) {};
  static void g() {};

  void h(this auto&&) {};
  static void h() {};
};

I wrote these tests expecting them to be ill-formed, and found what I
thought was a bug when they were not diagnosed as redecelarations.
However, given how the code for resolving overloads and determining
redeclarations looks, I believe this is actually well formed on a
technicality. I can't find the passages in the standard that specify
this so I can't be sure.

Anyway, the template parameter list differs because of the deduced
object parameter. Now here is the question, you are required to ignore
the object parameter when determining if these are redeclarations or
not, but what about the template parameters associated with the object
parameter? Am I just missing the passage that specifies this or is this
an actual defect in the standard?

The annoying thing is, even if this was brought up, I think the only
solution is to ratify these examples as well formed.

struct S {
  template<typename T>
  void f(this T, T) {}
  template<typename T>
  static void f(T) {}
};

Like what about this? If we ignore the template parameters associated
with the explicit object parameter, then the template parameter lists
don't match.

struct S {
  template<template<typename> typename Templ, typename T>
  void f(this Templ<T>, T) {}
  template<typename T>
  void f(T) {}
};

However, after writing them out and thinking about it a little, maybe
it really is just that simple. If after eliminating the template
parameters the explicit object parameter depends on the template
parameter lists are the same, then it's a redeclaration. Maybe this
works?

Am I overthinking this? Is there actually something specifying this
properly already? Hopefully that's the case but at the very least I
managed to write out this e-mail fairly quick for once so I didn't
waste too much time on this even if it does turn out to be nothing. The
majority of my time here was spent on the test case, which needed to be
written anyway.

Alex

On 12/1/23 01:02, waffl3x wrote:
> I ran into another issue while devising tests for redeclarations of
> xobj member functions as static member functions and vice versa. I am
> pretty sure by the literal wording of the standard, this is well formed.
> 
> template<typename T>
> concept Constrain = true;
> 
> struct S {
>    void f(this auto, Constrain auto) {};
>    static void f(Constrain auto) {};
> 
>    void g(this auto const&, Constrain auto) {};
>    static void g(Constrain auto) {};
> 
>    void h(this auto&&, Constrain auto) {};
>    static void h(Constrain auto) {};
> };
> 
> And also,
> 
> struct S{
>    void f(this auto) {};
>    static void f() {};
> 
>    void g(this auto const&) {};
>    static void g() {};
> 
>    void h(this auto&&) {};
>    static void h() {};
> };
> 
> I wrote these tests expecting them to be ill-formed, and found what I
> thought was a bug when they were not diagnosed as redecelarations.
> However, given how the code for resolving overloads and determining
> redeclarations looks, I believe this is actually well formed on a
> technicality. I can't find the passages in the standard that specify
> this so I can't be sure.

I think the relevant section is
https://eel.is/c++draft/basic.scope.scope

> Anyway, the template parameter list differs because of the deduced
> object parameter. Now here is the question, you are required to ignore
> the object parameter when determining if these are redeclarations or
> not, but what about the template parameters associated with the object
> parameter? Am I just missing the passage that specifies this or is this
> an actual defect in the standard?

I think that since they differ in template parameters, they don't 
correspond under https://eel.is/c++draft/basic.scope.scope#4.5 so they 
can be overloaded.

This is specified in terms of the template-head grammar non-terminal, 
but elsewhere we say that abbreviated templates are equivalent to 
writing out the template parameters explicitly.

> The annoying thing is, even if this was brought up, I think the only
> solution is to ratify these examples as well formed.

Yes.

Jason

On Friday, December 1st, 2023 at 9:52 AM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 12/1/23 01:02, waffl3x wrote:
> 
> > I ran into another issue while devising tests for redeclarations of
> > xobj member functions as static member functions and vice versa. I am
> > pretty sure by the literal wording of the standard, this is well formed.
> > 
> > template<typename T>
> > concept Constrain = true;
> > 
> > struct S {
> > void f(this auto, Constrain auto) {};
> > static void f(Constrain auto) {};
> > 
> > void g(this auto const&, Constrain auto) {};
> > static void g(Constrain auto) {};
> > 
> > void h(this auto&&, Constrain auto) {};
> > static void h(Constrain auto) {};
> > };
> > 
> > And also,
> > 
> > struct S{
> > void f(this auto) {};
> > static void f() {};
> > 
> > void g(this auto const&) {};
> > static void g() {};
> > 
> > void h(this auto&&) {};
> > static void h() {};
> > };
> > 
> > I wrote these tests expecting them to be ill-formed, and found what I
> > thought was a bug when they were not diagnosed as redecelarations.
> > However, given how the code for resolving overloads and determining
> > redeclarations looks, I believe this is actually well formed on a
> > technicality. I can't find the passages in the standard that specify
> > this so I can't be sure.
> 
> 
> I think the relevant section is
> https://eel.is/c++draft/basic.scope.scope
> 
> > Anyway, the template parameter list differs because of the deduced
> > object parameter. Now here is the question, you are required to ignore
> > the object parameter when determining if these are redeclarations or
> > not, but what about the template parameters associated with the object
> > parameter? Am I just missing the passage that specifies this or is this
> > an actual defect in the standard?
> 
> 
> I think that since they differ in template parameters, they don't
> correspond under https://eel.is/c++draft/basic.scope.scope#4.5 so they
> can be overloaded.
> 
> This is specified in terms of the template-head grammar non-terminal,
> but elsewhere we say that abbreviated templates are equivalent to
> writing out the template parameters explicitly.
> 
> > The annoying thing is, even if this was brought up, I think the only
> > solution is to ratify these examples as well formed.
> 
> 
> Yes.
> 
> Jason

I can't get over that I feel like this goes against the spirit of the
specification. Just because an object argument is deduced should not
suddenly mean we take it into account. Too bad there's no good solution.

I especially don't like that that the following case is ambiguous. I
understand why, but I don't like it.

template<typename T>
concept Constrain = true;

struct S {
  int f(this auto, Constrain auto) {};
  static f(auto) {};
};
main() {
  S{}.f(0);
}

I would like to see this changed honestly. When an ambiguity is
encountered, the more constrained function should be taken into account
even if they normally can't be considered. Is there some pitfall with
this line of thinking that kept it out of the standard? Is it just a
case of "too hard to specify" or is there some reason it's impossible
to do in all but the simplest of cases?

Anyway while I do think this behavior is bad (not wrong according to
the standard, but bad imo), I recognize I don't have time to think
about it right now so I'll go back to working on the patch for the time
being.

Alex

On 12/1/23 20:31, waffl3x wrote:
> On Friday, December 1st, 2023 at 9:52 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 12/1/23 01:02, waffl3x wrote:
>>
>>> I ran into another issue while devising tests for redeclarations of
>>> xobj member functions as static member functions and vice versa. I am
>>> pretty sure by the literal wording of the standard, this is well formed.
>>>
>>> template<typename T>
>>> concept Constrain = true;
>>>
>>> struct S {
>>> void f(this auto, Constrain auto) {};
>>> static void f(Constrain auto) {};
>>>
>>> void g(this auto const&, Constrain auto) {};
>>> static void g(Constrain auto) {};
>>>
>>> void h(this auto&&, Constrain auto) {};
>>> static void h(Constrain auto) {};
>>> };
>>>
>>> And also,
>>>
>>> struct S{
>>> void f(this auto) {};
>>> static void f() {};
>>>
>>> void g(this auto const&) {};
>>> static void g() {};
>>>
>>> void h(this auto&&) {};
>>> static void h() {};
>>> };
>>>
>>> I wrote these tests expecting them to be ill-formed, and found what I
>>> thought was a bug when they were not diagnosed as redecelarations.
>>> However, given how the code for resolving overloads and determining
>>> redeclarations looks, I believe this is actually well formed on a
>>> technicality. I can't find the passages in the standard that specify
>>> this so I can't be sure.
>>
>>
>> I think the relevant section is
>> https://eel.is/c++draft/basic.scope.scope
>>
>>> Anyway, the template parameter list differs because of the deduced
>>> object parameter. Now here is the question, you are required to ignore
>>> the object parameter when determining if these are redeclarations or
>>> not, but what about the template parameters associated with the object
>>> parameter? Am I just missing the passage that specifies this or is this
>>> an actual defect in the standard?
>>
>>
>> I think that since they differ in template parameters, they don't
>> correspond under https://eel.is/c++draft/basic.scope.scope#4.5 so they
>> can be overloaded.
>>
>> This is specified in terms of the template-head grammar non-terminal,
>> but elsewhere we say that abbreviated templates are equivalent to
>> writing out the template parameters explicitly.
>>
>>> The annoying thing is, even if this was brought up, I think the only
>>> solution is to ratify these examples as well formed.
>>
>> Yes.
> 
> I can't get over that I feel like this goes against the spirit of the
> specification. Just because an object argument is deduced should not
> suddenly mean we take it into account. Too bad there's no good solution.

Yep.  Note that it's normal for a template to overload with a non-template:

struct A
{
   void f();
   template <class T> void f();  // OK
};

> I especially don't like that that the following case is ambiguous. I
> understand why, but I don't like it.
> 
> template<typename T>
> concept Constrain = true;
> 
> struct S {
>    int f(this auto, Constrain auto) {};
>    static f(auto) {};
> };
> main() {
>    S{}.f(0);
> }
>
> I would like to see this changed honestly. When an ambiguity is
> encountered, the more constrained function should be taken into account
> even if they normally can't be considered. Is there some pitfall with
> this line of thinking that kept it out of the standard? Is it just a
> case of "too hard to specify" or is there some reason it's impossible
> to do in all but the simplest of cases?

I would actually expect the static function to be chosen as more 
specialized before we get to considering constraints, just as with

void f(auto, Constrain auto) = delete;
void f(const S&, auto) {}
int main() { f(S{},0); } // OK

Though it looks like [temp.func.order] needs to be adjusted for explicit 
object parameters.  And more_specialized_fn in gcc still has an outdated 
handling of object parameters that just skips them, from before the 
clearer specification in C++11 and later; this is PR53499.  No need to 
address that preexisting bug in this patch.

Jason

>> @@ -15402,6 +15450,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain,
>>                   gcc_checking_assert (TYPE_MAIN_VARIANT (TREE_TYPE (ve))
>>                                        == TYPE_MAIN_VARIANT (type));
>>                 SET_DECL_VALUE_EXPR (r, ve);
>> +               if (is_capture_proxy (t))
>> +                 type = TREE_TYPE (ve);

>That should have close to the same effect as the lambda_proxy_type
>adjustment I was talking about, since that function basically returns
>the TREE_TYPE of the COMPONENT_REF.  But the underlying problem is that
>finish_non_static_data_member assumes that 'object' is '*this', for
>which you can trust the cv-quals; for auto&&, you can't.
>capture_decltype has the same problem.  I'm attaching a patch to address
>this in both places.

Regarding this, was my change actually okay, and was your change
supposed to address it? I applied my patch to the latest commit in
master yesterday and started tests and whatnot with this change
commented out as I wasn't sure. It seems like my tests for constness of
captures no longer works with or without this change commented out.

If you wish I can go over everything again and figure out a new
solution with your changes but stepping through all this code was quite
a task that I'm weary of doing again. Even if the second time through
won't be so arduous I would like to avoid it.

You know what, I'll give it a go anyway but I don't want to spend too
much time on it, I still have a few tests to clean up and this crash to
fix.

template <class T> void f()
{
   int i;
   [=](this T&& self){ return i; }(); // error, unrelated
}
int main() { f<int>(); }

If this crash doesn't take too long (I don't think it will, it seems
straightforward enough) then I'll look at fixing the captures with a
const xobject parameter bug the correct way.

Alex

On Monday, December 4th, 2023 at 9:35 PM, waffl3x <waffl3x@protonmail.com> wrote:


> 
> 
> >> @@ -15402,6 +15450,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain,
> 
> > > gcc_checking_assert (TYPE_MAIN_VARIANT (TREE_TYPE (ve))
> > > == TYPE_MAIN_VARIANT (type));
> > > SET_DECL_VALUE_EXPR (r, ve);
> > > + if (is_capture_proxy (t))
> > > + type = TREE_TYPE (ve);
> 
> > That should have close to the same effect as the lambda_proxy_type
> > adjustment I was talking about, since that function basically returns
> > the TREE_TYPE of the COMPONENT_REF. But the underlying problem is that
> > finish_non_static_data_member assumes that 'object' is '*this', for
> > which you can trust the cv-quals; for auto&&, you can't.
> > capture_decltype has the same problem. I'm attaching a patch to address
> > this in both places.
> 
> 
> Regarding this, was my change actually okay, and was your change
> supposed to address it? I applied my patch to the latest commit in
> master yesterday and started tests and whatnot with this change
> commented out as I wasn't sure. It seems like my tests for constness of
> captures no longer works with or without this change commented out.
> 
> If you wish I can go over everything again and figure out a new
> solution with your changes but stepping through all this code was quite
> a task that I'm weary of doing again. Even if the second time through
> won't be so arduous I would like to avoid it.
> 
> You know what, I'll give it a go anyway but I don't want to spend too
> much time on it, I still have a few tests to clean up and this crash to
> fix.
> 
> template <class T> void f()
> 
> {
> int i;
> [=](this T&& self){ return i; }(); // error, unrelated
> }
> int main() { f<int>(); }
> 
> 
> If this crash doesn't take too long (I don't think it will, it seems
> straightforward enough) then I'll look at fixing the captures with a
> const xobject parameter bug the correct way.
> 
> Alex

WAIT Scratch that, I made a mistake, there's only a single case that is
broken, I read the test log wrong. Ah, I swear I'm cursed to realize
things the moment I hit the send button.

I have to take a closer look, I'll get back to you when I know more,
just trying to make sure you don't waste your time on this due to my
mistake.

Alex

On Monday, December 4th, 2023 at 9:39 PM, waffl3x <waffl3x@protonmail.com> wrote:

> On Monday, December 4th, 2023 at 9:35 PM, waffl3x waffl3x@protonmail.com wrote:
>
>
>
> > > > @@ -15402,6 +15450,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain,
> >
> > > > gcc_checking_assert (TYPE_MAIN_VARIANT (TREE_TYPE (ve))
> > > > == TYPE_MAIN_VARIANT (type));
> > > > SET_DECL_VALUE_EXPR (r, ve);
> > > > + if (is_capture_proxy (t))
> > > > + type = TREE_TYPE (ve);
> >
> > > That should have close to the same effect as the lambda_proxy_type
> > > adjustment I was talking about, since that function basically returns
> > > the TREE_TYPE of the COMPONENT_REF. But the underlying problem is that
> > > finish_non_static_data_member assumes that 'object' is '*this', for
> > > which you can trust the cv-quals; for auto&&, you can't.
> > > capture_decltype has the same problem. I'm attaching a patch to address
> > > this in both places.
> >
> > Regarding this, was my change actually okay, and was your change
> > supposed to address it? I applied my patch to the latest commit in
> > master yesterday and started tests and whatnot with this change
> > commented out as I wasn't sure. It seems like my tests for constness of
> > captures no longer works with or without this change commented out.
> >
> > If you wish I can go over everything again and figure out a new
> > solution with your changes but stepping through all this code was quite
> > a task that I'm weary of doing again. Even if the second time through
> > won't be so arduous I would like to avoid it.
> >
> > You know what, I'll give it a go anyway but I don't want to spend too
> > much time on it, I still have a few tests to clean up and this crash to
> > fix.
> >
> > template <class T> void f()
> >
> > {
> > int i;
> > [=](this T&& self){ return i; }(); // error, unrelated
> > }
> > int main() { f<int>(); }
> >
> > If this crash doesn't take too long (I don't think it will, it seems
> > straightforward enough) then I'll look at fixing the captures with a
> > const xobject parameter bug the correct way.
> >
> > Alex
>
>
> WAIT Scratch that, I made a mistake, there's only a single case that is
> broken, I read the test log wrong. Ah, I swear I'm cursed to realize
> things the moment I hit the send button.
>
> I have to take a closer look, I'll get back to you when I know more,
> just trying to make sure you don't waste your time on this due to my
> mistake.
>
> Alex

tl;dr it wasn't important, I just have to fix my test.

Okay that was faster than I anticipated, but unfortunately I don't know
how to handle it. I think your change in finish_non_static_data_member
might have been too heavy handed, but I don't know if there's a middle
ground. Or that's what I was going to say until I tested my assumption
on godbolt.

void f(auto const& a) { a = 5; }

Clang, MSVC and GCC all accept this until it is actually instantiated.

So, the true answer to my test failing is to just instantiate the
template. The test in question that was failing looks like this.

auto f2 = [n = 5](this auto const&){ n = 10; }; // { dg-error {} }

With the way things were before, this actually worked, so what my
assumption is now is that for us to actually diagnose this before a
template is instantiated would take some significant reworking of how
things are currently done. AND, I don't even know if it's legal for us
to make this diagnostic before instantiation for either of these cases.

Hah, come to think of it, we can't, there could be an overloaded
operator= that this is valid for... how disappointing.

We can for lambdas since the type is not dependent (on the lambda
instantiation) but it just isn't worth the effort I reckon.

Whatever, moving on, spending time on these things always drains me
because I think "oh boy I can do something better" and finding out it's
just not possible sucks. It's worse when it's because I overlooked
something that's obvious in hindsight.

Oh well, only that crash left I believe.

Alex

Here is the next version, it feels very close to finished. As before, I
haven't ran a bootstrap or the full testsuite yet but I did run the
explicit-obj tests which completed as expected.

There's a few test cases that still need to be written but more tests
can always be added. The behavior added by CWG2789 works in at least
one case, but I have not added tests for it yet. The test cases for
dependent lambda expressions need to be fleshed out more, but a few
temporary ones are included to demonstrate that they do work and that
the crash is fixed. Explicit object conversion functions work, but I
need to add fleshed out tests for them, explicit-obj-basic5.C has that
test.

I'll start the tests now and report back if anything fails, I'm
confident everything will be fine though.

Alex

On Wed, Dec 06, 2023 at 07:33:21AM +0000, waffl3x wrote:
> Here is the next version, it feels very close to finished. As before, I
> haven't ran a bootstrap or the full testsuite yet but I did run the
> explicit-obj tests which completed as expected.
> 
> There's a few test cases that still need to be written but more tests
> can always be added. The behavior added by CWG2789 works in at least
> one case, but I have not added tests for it yet. The test cases for
> dependent lambda expressions need to be fleshed out more, but a few
> temporary ones are included to demonstrate that they do work and that
> the crash is fixed. Explicit object conversion functions work, but I
> need to add fleshed out tests for them, explicit-obj-basic5.C has that
> test.
> 
> I'll start the tests now and report back if anything fails, I'm
> confident everything will be fine though.
> 
> Alex

> From 937e12c57145bfd878a0bc4cd9735c2d3c4fcf22 Mon Sep 17 00:00:00 2001
> From: Waffl3x <waffl3x@protonmail.com>
> Date: Tue, 5 Dec 2023 23:16:01 -0700
> Subject: [PATCH] P0847R7 (Deducing This) [PR102609] Another quick and dirty
>  patch for review, hopefully the last. gcc/cp/ChangeLog:
> 

Please add
	PR c++/102609
line above this.

> 	* call.cc (build_this_conversion):

Note, for the final submission, all the ):
should be followed by descriptions what has changed in there (but not why).
Plus it would be good to mention somewhere early in the cp/ChangeLog
entry that the patch implements C++23 P0847R7 - Deducing this paper
(unfortunately the ChangeLog verifier doesn't allow such free text above
the ChangeLog entry where it used to be written some years ago,
only allows there the PR line; I usually put such text after the ):
of the first entry now and only after it write exactly what changed
in that function.  Does the patch also implement CWG2586?

Also, I don't see in the patch the expected
gcc/c-family/
	* c-cppbuiltin.cc (c_cpp_builtins): Predefine
	__cpp_explicit_this_parameter=202110L for C++23.
part plus gcc/testsuite/cpp{23,26}/feat-cxx*.C additions checking
for that macro presence and its value.

	Jakub

On Wednesday, December 6th, 2023 at 1:48 AM, Jakub Jelinek <jakub@redhat.com> wrote:


> 
> 
> On Wed, Dec 06, 2023 at 07:33:21AM +0000, waffl3x wrote:
> 
> > Here is the next version, it feels very close to finished. As before, I
> > haven't ran a bootstrap or the full testsuite yet but I did run the
> > explicit-obj tests which completed as expected.
> > 
> > There's a few test cases that still need to be written but more tests
> > can always be added. The behavior added by CWG2789 works in at least
> > one case, but I have not added tests for it yet. The test cases for
> > dependent lambda expressions need to be fleshed out more, but a few
> > temporary ones are included to demonstrate that they do work and that
> > the crash is fixed. Explicit object conversion functions work, but I
> > need to add fleshed out tests for them, explicit-obj-basic5.C has that
> > test.
> > 
> > I'll start the tests now and report back if anything fails, I'm
> > confident everything will be fine though.
> > 
> > Alex
> 
> > From 937e12c57145bfd878a0bc4cd9735c2d3c4fcf22 Mon Sep 17 00:00:00 2001
> > From: Waffl3x waffl3x@protonmail.com
> > Date: Tue, 5 Dec 2023 23:16:01 -0700
> > Subject: [PATCH] P0847R7 (Deducing This) [PR102609] Another quick and dirty
> > patch for review, hopefully the last. gcc/cp/ChangeLog:
> 
> 
> Please add
> PR c++/102609
> line above this.
> 
> > * call.cc (build_this_conversion):
> 
> 
> Note, for the final submission, all the ):
> should be followed by descriptions what has changed in there (but not why).

Yeah, I remember the drill, it just takes me a long time so I've been
slacking.

> Plus it would be good to mention somewhere early in the cp/ChangeLog
> entry that the patch implements C++23 P0847R7 - Deducing this paper
> (unfortunately the ChangeLog verifier doesn't allow such free text above
> the ChangeLog entry where it used to be written some years ago,
> only allows there the PR line; I usually put such text after the ):
> of the first entry now and only after it write exactly what changed
> in that function. Does the patch also implement CWG2586?

Oh jeez, I had been doing it the way you're saying is rejected.
Shouldn't the ChangeLog verifier be changed to allow this?

The patch does not implement CWG2586 at this time. I couldn't determine
if it were ready to go or not. I have a skeleton of tests for it that I
never finished, but as far as I know the implementation does conform to
CWG2789, this just happened to be how it worked out.

> 
> Also, I don't see in the patch the expected
> gcc/c-family/
> * c-cppbuiltin.cc (c_cpp_builtins): Predefine
> __cpp_explicit_this_parameter=202110L for C++23.
> part plus gcc/testsuite/cpp{23,26}/feat-cxx*.C additions checking
> for that macro presence and its value.
> 
> Jakub

Yeah I was meaning to look into how to do that, I originally added the
test and then never included it in any of the patches, or that's what
remember anyway. This saves me the work though, I'll be sure to add
that.

Alex

Follow up to this, bootstrapped and tested with no regressions.

On Wednesday, December 6th, 2023 at 12:33 AM, waffl3x <waffl3x@protonmail.com> wrote:


> 
> 
> Here is the next version, it feels very close to finished. As before, I
> haven't ran a bootstrap or the full testsuite yet but I did run the
> explicit-obj tests which completed as expected.
> 
> There's a few test cases that still need to be written but more tests
> can always be added. The behavior added by CWG2789 works in at least
> one case, but I have not added tests for it yet. The test cases for
> dependent lambda expressions need to be fleshed out more, but a few
> temporary ones are included to demonstrate that they do work and that
> the crash is fixed. Explicit object conversion functions work, but I
> need to add fleshed out tests for them, explicit-obj-basic5.C has that
> test.
> 
> I'll start the tests now and report back if anything fails, I'm
> confident everything will be fine though.
> 
> Alex

On 12/6/23 02:33, waffl3x wrote:
> Here is the next version, it feels very close to finished. As before, I
> haven't ran a bootstrap or the full testsuite yet but I did run the
> explicit-obj tests which completed as expected.
> 
> There's a few test cases that still need to be written but more tests
> can always be added. The behavior added by CWG2789 works in at least
> one case, but I have not added tests for it yet. The test cases for
> dependent lambda expressions need to be fleshed out more, but a few
> temporary ones are included to demonstrate that they do work and that
> the crash is fixed. Explicit object conversion functions work, but I
> need to add fleshed out tests for them, explicit-obj-basic5.C has that
> test.

> @@ -6586,6 +6586,17 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
> +           /* FIXME: I believe this will be bugged for xobj member functions,
> +              leaving this comment here to make sure we look into it
> +              at some point.
> +              Seeing this makes me want correspondence checking to be unified
> +              in one place though, not sure if this one needs to be different
> +              from other ones though.
> +              This function is only used here, but maybe we can use it in add
> +              method and move some of the logic out of there?

fns_correspond absolutely needs updating to handle xob fns, and doing 
that by unifying it with add_method's calculation would be good.

> +              Side note: CWG2586 might be relevant for this area in
> +              particular, perhaps we wait to see if it gets accepted first?  */

2586 was accepted last year.

> @@ -12574,17 +12601,25 @@ cand_parms_match (z_candidate *c1, z_candidate *c2)
>        fn1 = DECL_TEMPLATE_RESULT (t1);
>        fn2 = DECL_TEMPLATE_RESULT (t2);
>      }
> +  /* The changes I made here might be stuff I was told not to worry about?
> +     I'm not really sure so I'm going to leave it in.  */

Good choice, this comment can go.

>    tree parms1 = TYPE_ARG_TYPES (TREE_TYPE (fn1));
>    tree parms2 = TYPE_ARG_TYPES (TREE_TYPE (fn2));
>    if (DECL_FUNCTION_MEMBER_P (fn1)
>        && DECL_FUNCTION_MEMBER_P (fn2)
> -      && (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn1)
> -         != DECL_NONSTATIC_MEMBER_FUNCTION_P (fn2)))
> +      && (DECL_STATIC_FUNCTION_P (fn1)
> +         != DECL_STATIC_FUNCTION_P (fn2)))
>      {
>        /* Ignore 'this' when comparing the parameters of a static member
>          function with those of a non-static one.  */
> -      parms1 = skip_artificial_parms_for (fn1, parms1);
> -      parms2 = skip_artificial_parms_for (fn2, parms2);
> +      auto skip_parms = [](tree fn, tree parms){
> +         if (DECL_XOBJ_MEMBER_FUNCTION_P (fn))
> +           return TREE_CHAIN (parms);
> +         else
> +           return skip_artificial_parms_for (fn, parms);
> +       };
> +      parms1 = skip_parms (fn1, parms1);
> +      parms2 = skip_parms (fn2, parms2);
>      }

https://cplusplus.github.io/CWG/issues/2789.html fixes the handling of 
xobj fns here.

Your change does the right thing for comparing static and xobj, but 
doesn't handle comparing iobj and xobj; I think we want to share 
parameter comparison code with fns_correspond/add_method.  Maybe 
parms_correspond?

> @@ -8727,21 +8882,42 @@ resolve_address_of_overloaded_function (tree target_type,
>    /* Good, exactly one match.  Now, convert it to the correct type.  */
>    fn = TREE_PURPOSE (matches);
>  
> -  if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
> -      && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
> +  if (DECL_OBJECT_MEMBER_FUNCTION_P (fn)
> +      && !(complain & tf_ptrmem_ok))
>      {
> -      static int explained;
> -
> -      if (!(complain & tf_error))
> +      /* For iobj member functions, if if -fms_extensions was passed in, this
> +        is not an error, so we do nothing.  It is still an error regardless
> +        for xobj member functions though, as it is a new feature we
> +        (hopefully) don't need to support the behavior.  */

Unfortunately, it seems that MSVC extended their weirdness to xobj fns, 
so -fms-extensions should as well.
https://godbolt.org/z/nfvn64Kx5

> +                 /* I'm keeping it more basic for now.  */

OK, this comment can go.

> @@ -15502,9 +15627,10 @@ void
>  grok_special_member_properties (tree decl)
>  {
>    tree class_type;
> -
> +  /* I believe we have to make some changes in here depending on the outcome
> +     of CWG2586.  */

As mentioned above, CWG2586 is resolved.  Be sure to scroll down to the 
approved resolution, or refer to the working draft.
https://cplusplus.github.io/CWG/issues/2586.html

> @@ -11754,8 +11754,16 @@ cp_parser_lambda_declarator_opt (cp_parser* parser, tre
>    else if (cxx_dialect < cxx23)
>      omitted_parms_loc = cp_lexer_peek_token (parser->lexer)->location;
>  
> +  /* Review note: I figured I might as well update the comments since I'm here.
> +     There are also some additions to the below.  */

Great, this comment can go.

> +      /* [expr.prim.lambda.general-4]
> +        If the lambda-declarator contains an explicit object parameter
> +        ([dcl.fct]), then no lambda-specifier in the lambda-specifier-seq
> +        shall be mutable or static.  */
> +      if (lambda_specs.storage_class == sc_mutable)
> +       {
> +         auto_diagnostic_group d;
> +         error_at (lambda_specs.locations[ds_storage_class],
> +                   "%<mutable%> lambda specifier "
> +                   "with explicit object parameter");
> +         /* Tell the user how to do what they probably meant, maybe fixits
> +            would be apropriate later?  */

"appropriate"

> +         if (!dependent_type_p (non_reference (param_type)))
> +           /* If we are given a non-dependent type we will have already given
> +              a diagnosis that the following would contradict with.  */;

Only if the lambda has captures, though?

We could also change dependent_type_p to the more specific 
WILDCARD_TYPE_P, I think, both here and just above.

> +         else if (!TYPE_REF_P (param_type))
> +           inform (DECL_SOURCE_LOCATION (xobj_param),
> +                   "the passed in closure object will not be mutated because "
> +                   "it is taken by copy/move");

"by value"

> @@ -3092,7 +3093,31 @@ finish_this_expr (void)
>      return rvalue (result);
>  
>    tree fn = current_nonlambda_function ();
> -  if (fn && DECL_STATIC_FUNCTION_P (fn))
> +  if (fn && DECL_XOBJ_MEMBER_FUNCTION_P (fn))
> +    {
> +      auto_diagnostic_group d;
> +      error ("%<this%> is unavailable for explicit object member "
> +            "functions");
> +      /* Doing a fixit here is possible, but hard, might be worthwhile
> +        in the future.  */
> +      tree xobj_parm = DECL_ARGUMENTS (fn);
> +      gcc_assert (xobj_parm);
> +      tree parm_name = DECL_NAME (xobj_parm);
> +      if (parm_name)
> +       inform (DECL_SOURCE_LOCATION (xobj_parm),
> +               "use explicit object parameter %qD instead",
> +               parm_name);
> +      else
> +       {
> +         gcc_rich_location xobj_loc (DECL_SOURCE_LOCATION (xobj_parm));
> +         /* This doesn't work and I don't know why.  I'll probably remove it
> +            before the final version.  */
> +         xobj_loc.add_fixit_insert_after (" self");
> +         inform (DECL_SOURCE_LOCATION (xobj_parm),
> +                 "name and use the explicit object parameter instead");

Now seems to be the time to either fix or remove it.

> @@ -12811,9 +12836,11 @@ capture_decltype (tree decl)
>        if (WILDCARD_TYPE_P (non_reference (obtype)))
>         /* We don't know what the eventual obtype quals will be.  */
>         return NULL_TREE;
> -      int quals = cp_type_quals (type);
> -      if (INDIRECT_TYPE_P (obtype))
> -       quals |= cp_type_quals (TREE_TYPE (obtype));
> +      /* This change possibly conflicts with another patch that is currently
> +        in flight. (Patrick Palka's PR83167 patch) I am just changing it for
> +        now to satisfy my tests.  */
> +      int const quals = cp_type_quals (type)
> +                     | cp_type_quals (TREE_TYPE (obtype));

Doesn't TREE_TYPE (obtype) break for by-value xob?  In that case I think 
we'd want cp_type_quals (obtype).

> +   The name "nonstatic" is no longer accurate with the addition of
> +   xobj member functions, should we change the name?  */
>  
>  bool
>  invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t complain)

This rule still applies to all non-static member functions:
https://eel.is/c++draft/expr.ref#6.3.2

> @@ -2352,7 +2355,7 @@ invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t complain)
>    if (is_overloaded_fn (expr) && !really_overloaded_fn (expr))
>      expr = get_first_fn (expr);
>    if (TREE_TYPE (expr)
> -      && DECL_NONSTATIC_MEMBER_FUNCTION_P (expr))
> +      && DECL_IOBJ_MEMBER_FUNCTION_P (expr))

...and so I think this should be _OBJECT_.
https://godbolt.org/z/r6v4e1ePP

Here's a patch to adjust all the remaining 
DECL_NONSTATIC_MEMBER_FUNCTION_P.  With this patch -diagnostic7.C gets 
the old address of non-static diagnostic, I think correctly, so I'm not 
sure we still need the BASELINK change in cp_build_addr_expr_1?

Jason

On Friday, December 8th, 2023 at 12:25 PM, Jason Merrill <jason@redhat.com> wrote:
> 
> 
> On 12/6/23 02:33, waffl3x wrote:
> 
> > Here is the next version, it feels very close to finished. As before, I
> > haven't ran a bootstrap or the full testsuite yet but I did run the
> > explicit-obj tests which completed as expected.
> > 
> > There's a few test cases that still need to be written but more tests
> > can always be added. The behavior added by CWG2789 works in at least
> > one case, but I have not added tests for it yet. The test cases for
> > dependent lambda expressions need to be fleshed out more, but a few
> > temporary ones are included to demonstrate that they do work and that
> > the crash is fixed. Explicit object conversion functions work, but I
> > need to add fleshed out tests for them, explicit-obj-basic5.C has that
> > test.
> 
> > @@ -6586,6 +6586,17 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> args,
> > + / FIXME: I believe this will be bugged for xobj member functions,
> > + leaving this comment here to make sure we look into it
> > + at some point.
> > + Seeing this makes me want correspondence checking to be unified
> > + in one place though, not sure if this one needs to be different
> > + from other ones though.
> > + This function is only used here, but maybe we can use it in add
> > + method and move some of the logic out of there?
> 
> 
> fns_correspond absolutely needs updating to handle xob fns, and doing
> that by unifying it with add_method's calculation would be good.
> 
> > + Side note: CWG2586 might be relevant for this area in
> > + particular, perhaps we wait to see if it gets accepted first? */
> 
> 
> 2586 was accepted last year.
> 
> > @@ -12574,17 +12601,25 @@ cand_parms_match (z_candidate *c1, z_candidate c2)
> > fn1 = DECL_TEMPLATE_RESULT (t1);
> > fn2 = DECL_TEMPLATE_RESULT (t2);
> > }
> > + / The changes I made here might be stuff I was told not to worry about?
> > + I'm not really sure so I'm going to leave it in. */
> 
> 
> Good choice, this comment can go.
> 
> > tree parms1 = TYPE_ARG_TYPES (TREE_TYPE (fn1));
> > tree parms2 = TYPE_ARG_TYPES (TREE_TYPE (fn2));
> > if (DECL_FUNCTION_MEMBER_P (fn1)
> > && DECL_FUNCTION_MEMBER_P (fn2)
> > - && (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn1)
> > - != DECL_NONSTATIC_MEMBER_FUNCTION_P (fn2)))
> > + && (DECL_STATIC_FUNCTION_P (fn1)
> > + != DECL_STATIC_FUNCTION_P (fn2)))
> > {
> > /* Ignore 'this' when comparing the parameters of a static member
> > function with those of a non-static one. */
> > - parms1 = skip_artificial_parms_for (fn1, parms1);
> > - parms2 = skip_artificial_parms_for (fn2, parms2);
> > + auto skip_parms = [](tree fn, tree parms){
> > + if (DECL_XOBJ_MEMBER_FUNCTION_P (fn))
> > + return TREE_CHAIN (parms);
> > + else
> > + return skip_artificial_parms_for (fn, parms);
> > + };
> > + parms1 = skip_parms (fn1, parms1);
> > + parms2 = skip_parms (fn2, parms2);
> > }
> 
> 
> https://cplusplus.github.io/CWG/issues/2789.html fixes the handling of
> xobj fns here.

I have a minor concern here.
https://eel.is/c++draft/basic.scope.scope#3

Is it okay that CWG2789 has separate rules than the rules for
declarations? As far as I know there's nothing else that describes how
to handle the object parameters so it was my assumption that the rules
here are also used for that. I know at least one person has disagreed
with me about that so I'm not sure what is actually correct.

template <typename T = int>
struct S {
  constexpr void f() {}                       // #f1
  constexpr void f(this S&&) requires true {} // #f2

  constexpr void g() requires true {}         // #g1
  constexpr void g(this S&&) {}               // #g2
};

void test() {
  S<>{}.f();             // calls #?
  S<>{}.g();             // calls #?
}

But with the wording proposed by CWG2789, wouldn't this example would
be a problem? If we follow the standard to the letter, constraints
can't be applied here right?

I wouldn't be surprised if I'm missing something but I figured I ought
to raise it just in case. Obviously it should call #f2 and #g1 but I'm
pretty sure the current wording only allows these calls to be ambiguous.

> Your change does the right thing for comparing static and xobj, but
> doesn't handle comparing iobj and xobj; I think we want to share
> parameter comparison code with fns_correspond/add_method. Maybe
> parms_correspond?

Yeah, I'll see what I can put together. The only issue being what I
noted above, I'm not sure the rules are actually the same. I think they
should be, but my reading of things seems like it's not right now.

For the time being I'm going to assume things should work the way I
want them to, because I don't think the example I presented above
should be ambiguous.

> > @@ -8727,21 +8882,42 @@ resolve_address_of_overloaded_function (tree target_type,
> > /* Good, exactly one match. Now, convert it to the correct type. */
> > fn = TREE_PURPOSE (matches);
> > 
> > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
> > - && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
> > + if (DECL_OBJECT_MEMBER_FUNCTION_P (fn)
> > + && !(complain & tf_ptrmem_ok))
> > {
> > - static int explained;
> > -
> > - if (!(complain & tf_error))
> > + /* For iobj member functions, if if -fms_extensions was passed in, this
> > + is not an error, so we do nothing. It is still an error regardless
> > + for xobj member functions though, as it is a new feature we
> > + (hopefully) don't need to support the behavior. */
> 
> 
> Unfortunately, it seems that MSVC extended their weirdness to xobj fns,
> so -fms-extensions should as well.
> https://godbolt.org/z/nfvn64Kx5

Ugh, what a shame. We don't have to support it though do we? The
documentation for -fms-extensions seems to state that it's for
microsoft headers, if they aren't using xobj parameters in their
headers as of now, and I can't see them doing so for the near future,
it should be fine to leave this out still shouldn't it?

The code will be simpler if we can't though so I reckon it's win/win
whichever way we choose.

> > + /* I'm keeping it more basic for now. */
> 
> 
> OK, this comment can go.

Yeah, if I ever get comfortable with fixit's I'll do a loop through my
changes and see where I can enhance the ones already present and add
the ones that I wanted.

> > @@ -15502,9 +15627,10 @@ void
> > grok_special_member_properties (tree decl)
> > {
> > tree class_type;
> > -
> > + /* I believe we have to make some changes in here depending on the outcome
> > + of CWG2586. */
> 
> 
> As mentioned above, CWG2586 is resolved. Be sure to scroll down to the
> approved resolution, or refer to the working draft.
> https://cplusplus.github.io/CWG/issues/2586.html

Yeah I got a bit of a lecture on how to read these in IRC, I shouldn't
be making this mistake again. :'D

I'll look at supporting it, I imagine it shouldn't be too hard but I've
been wrong before.

> > @@ -11754,8 +11754,16 @@ cp_parser_lambda_declarator_opt (cp_parser* parser, tre
> > else if (cxx_dialect < cxx23)
> > omitted_parms_loc = cp_lexer_peek_token (parser->lexer)->location;
> > 
> > + /* Review note: I figured I might as well update the comments since I'm here.
> > + There are also some additions to the below. */
> 
> 
> Great, this comment can go.

Just noting, I'm also removing the old comment immediately following
this one as it is obsolete.

> > + /* [expr.prim.lambda.general-4]
> > + If the lambda-declarator contains an explicit object parameter
> > + ([dcl.fct]), then no lambda-specifier in the lambda-specifier-seq
> > + shall be mutable or static. /
> > + if (lambda_specs.storage_class == sc_mutable)
> > + {
> > + auto_diagnostic_group d;
> > + error_at (lambda_specs.locations[ds_storage_class],
> > + "%<mutable%> lambda specifier "
> > + "with explicit object parameter");
> > + / Tell the user how to do what they probably meant, maybe fixits
> > + would be apropriate later? */
> 
> 
> "appropriate"
> 
> > + if (!dependent_type_p (non_reference (param_type)))
> > + /* If we are given a non-dependent type we will have already given
> > + a diagnosis that the following would contradict with. */;
> 
> 
> Only if the lambda has captures, though?

Oops, yeah I agree.

> We could also change dependent_type_p to the more specific
> WILDCARD_TYPE_P, I think, both here and just above.

I don't understand the significance of this but I'll trust there is
one. Better to be consistent with your other changes anyway.

> > + else if (!TYPE_REF_P (param_type))
> > + inform (DECL_SOURCE_LOCATION (xobj_param),
> > + "the passed in closure object will not be mutated because "
> > + "it is taken by copy/move");
> 
> 
> "by value"
> 
> > @@ -3092,7 +3093,31 @@ finish_this_expr (void)
> > return rvalue (result);
> > 
> > tree fn = current_nonlambda_function ();
> > - if (fn && DECL_STATIC_FUNCTION_P (fn))
> > + if (fn && DECL_XOBJ_MEMBER_FUNCTION_P (fn))
> > + {
> > + auto_diagnostic_group d;
> > + error ("%<this%> is unavailable for explicit object member "
> > + "functions");
> > + /* Doing a fixit here is possible, but hard, might be worthwhile
> > + in the future. /
> > + tree xobj_parm = DECL_ARGUMENTS (fn);
> > + gcc_assert (xobj_parm);
> > + tree parm_name = DECL_NAME (xobj_parm);
> > + if (parm_name)
> > + inform (DECL_SOURCE_LOCATION (xobj_parm),
> > + "use explicit object parameter %qD instead",
> > + parm_name);
> > + else
> > + {
> > + gcc_rich_location xobj_loc (DECL_SOURCE_LOCATION (xobj_parm));
> > + / This doesn't work and I don't know why. I'll probably remove it
> > + before the final version. */
> > + xobj_loc.add_fixit_insert_after (" self");
> > + inform (DECL_SOURCE_LOCATION (xobj_parm),
> > + "name and use the explicit object parameter instead");
> 
> 
> Now seems to be the time to either fix or remove it.

I think I'll defer it to future work, I'll pull it from the next
version. Fixits are arcane, I'll have to do a deep dive on them to be
able to use them properly I think. I'll also get in touch with David
Malcolm as you suggested and see what I can learn about them from him.

> > @@ -12811,9 +12836,11 @@ capture_decltype (tree decl)
> > if (WILDCARD_TYPE_P (non_reference (obtype)))
> > /* We don't know what the eventual obtype quals will be. /
> > return NULL_TREE;
> > - int quals = cp_type_quals (type);
> > - if (INDIRECT_TYPE_P (obtype))
> > - quals |= cp_type_quals (TREE_TYPE (obtype));
> > + / This change possibly conflicts with another patch that is currently
> > + in flight. (Patrick Palka's PR83167 patch) I am just changing it for
> > + now to satisfy my tests. */
> > + int const quals = cp_type_quals (type)
> > + | cp_type_quals (TREE_TYPE (obtype));
> 
> 
> Doesn't TREE_TYPE (obtype) break for by-value xob? In that case I think
> we'd want cp_type_quals (obtype).

I feel like you're right, and I'm disappointed that I missed this case
in my tests. Indeed, I've checked and it ICEs for by-value xobj
parameters. I'll fix this and add the test case.

> > + The name "nonstatic" is no longer accurate with the addition of
> > + xobj member functions, should we change the name? */
> > 
> > bool
> > invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t complain)
> 
> 
> This rule still applies to all non-static member functions:
> https://eel.is/c++draft/expr.ref#6.3.2

Alright I see where my mistake is now, the comment regarding .* and ->*
threw me for a loop. Even so, I figured since we renamed
DECL_NONSTATIC_MEMBER_FUNCTION_P we might be better off renaming other
things too. So in this case, invalid_object_memfn_p instead of
invalid_nonstatic_memfn_p.

Obviously changing the errors and warnings is a different beast and
probably should be left alone.

> > @@ -2352,7 +2355,7 @@ invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t complain)
> > if (is_overloaded_fn (expr) && !really_overloaded_fn (expr))
> > expr = get_first_fn (expr);
> > if (TREE_TYPE (expr)
> > - && DECL_NONSTATIC_MEMBER_FUNCTION_P (expr))
> > + && DECL_IOBJ_MEMBER_FUNCTION_P (expr))
> 
> 
> ...and so I think this should be OBJECT.
> https://godbolt.org/z/r6v4e1ePP

Yeah, I agree, definitely a mistake on my part.

Since we are here, following the same logic I presented above, can we
refuse to support -fms-extensions for xobj member functions? Until
microsoft is using xobj member functions in their headers we shouldn't
need to support it right?

Supporting their non-conformance going forward doesn't make sense to
me. Especially since the non-conformance with xobj member functions is
likely just because it was already this way. Until we know for sure
that they rely on this behavior for xobj member functions I think it
should remain off, it's a simple change to add it back in after all.

> Here's a patch to adjust all the remaining
> DECL_NONSTATIC_MEMBER_FUNCTION_P. With this patch -diagnostic7.C gets
> the old address of non-static diagnostic, I think correctly, so I'm not
> sure we still need the BASELINK change in cp_build_addr_expr_1?
> 
> Jason

Thanks, I'll evaluate the BASELINK change to see if you're right.
Hopefully you are, that would simplify things.

Is there anything special I need to do when adding this patch? I was
worried I'm supposed to maintain it's origin in it's own commit or
something. Well, with that said it's probably still something I should
learn to do anyway, I'm just trying really hard to put it off until the
patch is done.

Alex

On 12/10/23 10:22, waffl3x wrote:
> On Friday, December 8th, 2023 at 12:25 PM, Jason Merrill <jason@redhat.com> wrote:
>>
>>
>> On 12/6/23 02:33, waffl3x wrote:
>>
>>> Here is the next version, it feels very close to finished. As before, I
>>> haven't ran a bootstrap or the full testsuite yet but I did run the
>>> explicit-obj tests which completed as expected.
>>>
>>> There's a few test cases that still need to be written but more tests
>>> can always be added. The behavior added by CWG2789 works in at least
>>> one case, but I have not added tests for it yet. The test cases for
>>> dependent lambda expressions need to be fleshed out more, but a few
>>> temporary ones are included to demonstrate that they do work and that
>>> the crash is fixed. Explicit object conversion functions work, but I
>>> need to add fleshed out tests for them, explicit-obj-basic5.C has that
>>> test.
>>
>>> @@ -6586,6 +6586,17 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> args,
>>> + / FIXME: I believe this will be bugged for xobj member functions,
>>> + leaving this comment here to make sure we look into it
>>> + at some point.
>>> + Seeing this makes me want correspondence checking to be unified
>>> + in one place though, not sure if this one needs to be different
>>> + from other ones though.
>>> + This function is only used here, but maybe we can use it in add
>>> + method and move some of the logic out of there?
>>
>>
>> fns_correspond absolutely needs updating to handle xob fns, and doing
>> that by unifying it with add_method's calculation would be good.
>>
>>> + Side note: CWG2586 might be relevant for this area in
>>> + particular, perhaps we wait to see if it gets accepted first? */
>>
>>
>> 2586 was accepted last year.
>>
>>> @@ -12574,17 +12601,25 @@ cand_parms_match (z_candidate *c1, z_candidate c2)
>>> fn1 = DECL_TEMPLATE_RESULT (t1);
>>> fn2 = DECL_TEMPLATE_RESULT (t2);
>>> }
>>> + / The changes I made here might be stuff I was told not to worry about?
>>> + I'm not really sure so I'm going to leave it in. */
>>
>>
>> Good choice, this comment can go.
>>
>>> tree parms1 = TYPE_ARG_TYPES (TREE_TYPE (fn1));
>>> tree parms2 = TYPE_ARG_TYPES (TREE_TYPE (fn2));
>>> if (DECL_FUNCTION_MEMBER_P (fn1)
>>> && DECL_FUNCTION_MEMBER_P (fn2)
>>> - && (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn1)
>>> - != DECL_NONSTATIC_MEMBER_FUNCTION_P (fn2)))
>>> + && (DECL_STATIC_FUNCTION_P (fn1)
>>> + != DECL_STATIC_FUNCTION_P (fn2)))
>>> {
>>> /* Ignore 'this' when comparing the parameters of a static member
>>> function with those of a non-static one. */
>>> - parms1 = skip_artificial_parms_for (fn1, parms1);
>>> - parms2 = skip_artificial_parms_for (fn2, parms2);
>>> + auto skip_parms = [](tree fn, tree parms){
>>> + if (DECL_XOBJ_MEMBER_FUNCTION_P (fn))
>>> + return TREE_CHAIN (parms);
>>> + else
>>> + return skip_artificial_parms_for (fn, parms);
>>> + };
>>> + parms1 = skip_parms (fn1, parms1);
>>> + parms2 = skip_parms (fn2, parms2);
>>> }
>>
>>
>> https://cplusplus.github.io/CWG/issues/2789.html fixes the handling of
>> xobj fns here.
> 
> I have a minor concern here.
> https://eel.is/c++draft/basic.scope.scope#3
> 
> Is it okay that CWG2789 has separate rules than the rules for
> declarations? As far as I know there's nothing else that describes how
> to handle the object parameters so it was my assumption that the rules
> here are also used for that. I know at least one person has disagreed
> with me about that so I'm not sure what is actually correct.
> 
> template <typename T = int>
> struct S {
>    constexpr void f() {}                       // #f1
>    constexpr void f(this S&&) requires true {} // #f2
> 
>    constexpr void g() requires true {}         // #g1
>    constexpr void g(this S&&) {}               // #g2
> };
> 
> void test() {
>    S<>{}.f();             // calls #?
>    S<>{}.g();             // calls #?
> }
> 
> But with the wording proposed by CWG2789, wouldn't this example would
> be a problem? If we follow the standard to the letter, constraints
> can't be applied here right?
> 
> I wouldn't be surprised if I'm missing something but I figured I ought
> to raise it just in case. Obviously it should call #f2 and #g1 but I'm
> pretty sure the current wording only allows these calls to be ambiguous.

I agree, I've suggested amending 2789 to use "corresponding object 
parameter".

>>> @@ -8727,21 +8882,42 @@ resolve_address_of_overloaded_function (tree target_type,
>>> /* Good, exactly one match. Now, convert it to the correct type. */
>>> fn = TREE_PURPOSE (matches);
>>>
>>> - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
>>> - && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
>>> + if (DECL_OBJECT_MEMBER_FUNCTION_P (fn)
>>> + && !(complain & tf_ptrmem_ok))
>>> {
>>> - static int explained;
>>> -
>>> - if (!(complain & tf_error))
>>> + /* For iobj member functions, if if -fms_extensions was passed in, this
>>> + is not an error, so we do nothing. It is still an error regardless
>>> + for xobj member functions though, as it is a new feature we
>>> + (hopefully) don't need to support the behavior. */
>>
>>
>> Unfortunately, it seems that MSVC extended their weirdness to xobj fns,
>> so -fms-extensions should as well.
>> https://godbolt.org/z/nfvn64Kx5
> 
> Ugh, what a shame. We don't have to support it though do we? The
> documentation for -fms-extensions seems to state that it's for
> microsoft headers, if they aren't using xobj parameters in their
> headers as of now, and I can't see them doing so for the near future,
> it should be fine to leave this out still shouldn't it?
> 
> The code will be simpler if we can't though so I reckon it's win/win
> whichever way we choose.

I think let's go with the simpler code, it doesn't seem particularly 
useful to split hairs on this obscure option.

>> We could also change dependent_type_p to the more specific
>> WILDCARD_TYPE_P, I think, both here and just above.
> 
> I don't understand the significance of this but I'll trust there is
> one. Better to be consistent with your other changes anyway.

FYI a "wildcard" type is e.g. a template type parm, typename, or 
decltype: types that could be replaced by a type of any form (and 
qualification), unlike e.g. A<T> which is also dependent, but won't turn 
const at instantiation time (if A is a class template).

>>> + The name "nonstatic" is no longer accurate with the addition of
>>> + xobj member functions, should we change the name? */
>>>
>>> bool
>>> invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t complain)
>>
>>
>> This rule still applies to all non-static member functions:
>> https://eel.is/c++draft/expr.ref#6.3.2
> 
> Alright I see where my mistake is now, the comment regarding .* and ->*
> threw me for a loop. Even so, I figured since we renamed
> DECL_NONSTATIC_MEMBER_FUNCTION_P we might be better off renaming other
> things too. So in this case, invalid_object_memfn_p instead of
> invalid_nonstatic_memfn_p.
> 
> Obviously changing the errors and warnings is a different beast and
> probably should be left alone.

I wouldn't bother renaming other things unnecessarily, it just seemed 
important to rename the predicate because it was being used for what are 
now two different meanings.

>> Here's a patch to adjust all the remaining
>> DECL_NONSTATIC_MEMBER_FUNCTION_P. With this patch -diagnostic7.C gets
>> the old address of non-static diagnostic, I think correctly, so I'm not
>> sure we still need the BASELINK change in cp_build_addr_expr_1?
> 
> Thanks, I'll evaluate the BASELINK change to see if you're right.
> Hopefully you are, that would simplify things.
> 
> Is there anything special I need to do when adding this patch? I was
> worried I'm supposed to maintain it's origin in it's own commit or
> something. Well, with that said it's probably still something I should
> learn to do anyway, I'm just trying really hard to put it off until the
> patch is done.

It's OK to integrate it into your patch if that's easier for you.

Jason

int n = 0;
  auto f = []<typename Self>(this Self){
    static_assert(__is_same (decltype(n), int));
    decltype((n)) a; // { dg-error {is not captured} }
  };
  f();

Could you clarify if this error being removed was intentional. I do
recall that Patrick Palka wanted to remove this error in his patch, but
it seemed to me like you stated it would be incorrect to allow it.
Since the error is no longer present I assume I am misunderstanding the
exchange.

In any case, let me know if I need to modify my test case or if this
error needs to be added back in.

Alex

On 12/22/23 04:01, waffl3x wrote:
> 
>    int n = 0;
>    auto f = []<typename Self>(this Self){
>      static_assert(__is_same (decltype(n), int));
>      decltype((n)) a; // { dg-error {is not captured} }
>    };
>    f();
> 
> Could you clarify if this error being removed was intentional. I do
> recall that Patrick Palka wanted to remove this error in his patch, but
> it seemed to me like you stated it would be incorrect to allow it.
> Since the error is no longer present I assume I am misunderstanding the
> exchange.
> 
> In any case, let me know if I need to modify my test case or if this
> error needs to be added back in.

Removing the error was correct under
https://eel.is/c++draft/expr.prim#id.unqual-3
Naming n in that lambda would not refer a capture by copy, so the 
decltype is the same as outside the lambda.

Jason

On Friday, December 22nd, 2023 at 10:26 AM, Jason Merrill <jason@redhat.com> wrote:
> 
> 
> On 12/22/23 04:01, waffl3x wrote:
> 
> > int n = 0;
> > auto f = []<typename Self>(this Self){
> > static_assert(__is_same (decltype(n), int));
> > decltype((n)) a; // { dg-error {is not captured} }
> > };
> > f();
> > 
> > Could you clarify if this error being removed was intentional. I do
> > recall that Patrick Palka wanted to remove this error in his patch, but
> > it seemed to me like you stated it would be incorrect to allow it.
> > Since the error is no longer present I assume I am misunderstanding the
> > exchange.
> > 
> > In any case, let me know if I need to modify my test case or if this
> > error needs to be added back in.
> 
> 
> Removing the error was correct under
> https://eel.is/c++draft/expr.prim#id.unqual-3
> Naming n in that lambda would not refer a capture by copy, so the
> decltype is the same as outside the lambda.
> 
> Jason

Alright, I've fixed my tests to reflect that.

I've got defaulting assignment operators working. Defaulting equality
and comparison operators seemed to work out of the box somehow, so I
just have to make some fleshed out tests for those cases.

There can always be more tests, I have a few ideas for what still needs
to be covered, mostly with dependent lambdas. Tests for xobj conversion
operators definitely need to be more fleshed out. I also need to
formulate some tests to make sure constraints are not being taking into
account when the object parameters should not correspond, but that's a
little more tough to test for than the valid cases.

Other than tests though, is there anything you can think of that the
patch is missing? Other than the aforementioned tests, I'm pretty
confident everything is done.

To recap, I have CWG2789 implemented on my end with the change we
discussed to require corresponding object parameters instead of the
same type, and I have CWG2586 implemented. I can't recall what other
outstanding issues we had, and my notes don't mention anything other
than tests. So I'm assuming everything is good.

Alex

On 12/23/23 02:10, waffl3x wrote:
> On Friday, December 22nd, 2023 at 10:26 AM, Jason Merrill <jason@redhat.com> wrote:
>>
>>
>> On 12/22/23 04:01, waffl3x wrote:
>>
>>> int n = 0;
>>> auto f = []<typename Self>(this Self){
>>> static_assert(__is_same (decltype(n), int));
>>> decltype((n)) a; // { dg-error {is not captured} }
>>> };
>>> f();
>>>
>>> Could you clarify if this error being removed was intentional. I do
>>> recall that Patrick Palka wanted to remove this error in his patch, but
>>> it seemed to me like you stated it would be incorrect to allow it.
>>> Since the error is no longer present I assume I am misunderstanding the
>>> exchange.
>>>
>>> In any case, let me know if I need to modify my test case or if this
>>> error needs to be added back in.
>>
>>
>> Removing the error was correct under
>> https://eel.is/c++draft/expr.prim#id.unqual-3
>> Naming n in that lambda would not refer a capture by copy, so the
>> decltype is the same as outside the lambda.
> 
> Alright, I've fixed my tests to reflect that.
> 
> I've got defaulting assignment operators working. Defaulting equality
> and comparison operators seemed to work out of the box somehow, so I
> just have to make some fleshed out tests for those cases.
> 
> There can always be more tests, I have a few ideas for what still needs
> to be covered, mostly with dependent lambdas. Tests for xobj conversion
> operators definitely need to be more fleshed out. I also need to
> formulate some tests to make sure constraints are not being taking into
> account when the object parameters should not correspond, but that's a
> little more tough to test for than the valid cases.
> 
> Other than tests though, is there anything you can think of that the
> patch is missing? Other than the aforementioned tests, I'm pretty
> confident everything is done.
> 
> To recap, I have CWG2789 implemented on my end with the change we
> discussed to require corresponding object parameters instead of the
> same type, and I have CWG2586 implemented. I can't recall what other
> outstanding issues we had, and my notes don't mention anything other
> than tests. So I'm assuming everything is good.

Sounds good!  Did you mean to include the updated patch?

Jason

I've been at this for a while, and I'm not sure what the proper way to
fix this is.

```
struct S;

struct B {
  void f(this S&) {}
  void g() {}
};

struct S : B {
  using B::f;
  using B::g;
  void f() {}
  void g(this S&) {}
};

int main()
{
  S s{};
  s.f();
  s.g();
}
```

In short, the call to f is ambiguous, but the call to g is not. I
already know where the problem is, but since I share this code in
places that don't know about whether a function was introduced by a
using declaration (cand_parms_match), I don't want to rely on that to
solve the problem.

```
  /* An iobj member function's object parameter can't be an unrelated type, if
     the xobj member function's object parameter is an unrelated type we know
     they must not correspond to each other.  If the iobj member function was
     introduced with a using declaration, then the type of its object parameter
     is still that of the class we are currently adding a member function to,
     so this assumption holds true in that case as well.

     [over.match.funcs.general.4]
     For non-conversion functions that are implicit object member
     functions nominated by a using-declaration in a derived class, the
     function is considered to be a member of the derived class for the purpose
     of defining the type of the implicit object parameter.

     We don't get to bail yet out even if the xobj parameter is by-value as
     elaborated on below.

     This also implicitly handles xobj parameters of type pointer.  */
  if (DECL_CONTEXT (xobj_fn) != TYPE_MAIN_VARIANT (non_reference (xobj_param)))
    return false;
```

I feel like what we are actually supposed to be doing to be to the
letter of the standard is to be creating a new function entirely, with
a decl_context of the original class, which sounds omega silly, and
might bring a new set of problems.

I think I might have came up with an unfortunately fairly convoluted
way to solve this just now, but I don't know if it brings another new
set of problems. The assumptions I had when I originally implemented
this in add_method bled through when I broke it out into it's own
function. At the very least I need to better document how the function
is intended to be used, at worst I'll need to consider whether it makes
sense to be reusing this logic if the use cases are subtly different.

I don't think the latter is the case now though, I'm noticing GCC just
has a bug in general with constraints and using declarations.

https://godbolt.org/z/EbGvjfG7E

So it might actually just be better to be rewriting functions that are
introduced by using declarations, I have a feeling that will be what
introduces the least pain.

I'm not sure where exactly GCC is deciding that a function introduced
by a using declaration is different from an otherwise corresponding one
declared directly in that class, but I have a feeling on where it is.
Obviously it's in joust, but I'm not sure the object parameters are
actually being compared.

I'll investigate this bug and get back to you, I imagine fixing it is
going to be key to actually implementing the xobj case without hacks.

Finding both these issues has slowed down my next revision as I noticed
the problem while cleaning up my implementation of CWG2789. I want to
note, I am implementing it as if it specifies corresponding object
arguments, not object arguments of the same type, as we previously
discussed, I believe that to be the right resolution as there are
really bad edge cases with the current wording.

Alex

On Tuesday, December 26th, 2023 at 9:37 AM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 12/23/23 02:10, waffl3x wrote:
> 
> > On Friday, December 22nd, 2023 at 10:26 AM, Jason Merrill jason@redhat.com wrote:
> > 
> > > On 12/22/23 04:01, waffl3x wrote:
> > > 
> > > > int n = 0;
> > > > auto f = []<typename Self>(this Self){
> > > > static_assert(__is_same (decltype(n), int));
> > > > decltype((n)) a; // { dg-error {is not captured} }
> > > > };
> > > > f();
> > > > 
> > > > Could you clarify if this error being removed was intentional. I do
> > > > recall that Patrick Palka wanted to remove this error in his patch, but
> > > > it seemed to me like you stated it would be incorrect to allow it.
> > > > Since the error is no longer present I assume I am misunderstanding the
> > > > exchange.
> > > > 
> > > > In any case, let me know if I need to modify my test case or if this
> > > > error needs to be added back in.
> > > 
> > > Removing the error was correct under
> > > https://eel.is/c++draft/expr.prim#id.unqual-3
> > > Naming n in that lambda would not refer a capture by copy, so the
> > > decltype is the same as outside the lambda.
> > 
> > Alright, I've fixed my tests to reflect that.
> > 
> > I've got defaulting assignment operators working. Defaulting equality
> > and comparison operators seemed to work out of the box somehow, so I
> > just have to make some fleshed out tests for those cases.
> > 
> > There can always be more tests, I have a few ideas for what still needs
> > to be covered, mostly with dependent lambdas. Tests for xobj conversion
> > operators definitely need to be more fleshed out. I also need to
> > formulate some tests to make sure constraints are not being taking into
> > account when the object parameters should not correspond, but that's a
> > little more tough to test for than the valid cases.
> > 
> > Other than tests though, is there anything you can think of that the
> > patch is missing? Other than the aforementioned tests, I'm pretty
> > confident everything is done.
> > 
> > To recap, I have CWG2789 implemented on my end with the change we
> > discussed to require corresponding object parameters instead of the
> > same type, and I have CWG2586 implemented. I can't recall what other
> > outstanding issues we had, and my notes don't mention anything other
> > than tests. So I'm assuming everything is good.
> 
> 
> Sounds good! Did you mean to include the updated patch?
> 
> Jason

That was faster than I expected, the problem is exactly just that we
aren't implementing [over.match.funcs.general.4] at all. The result of
compparms for the 2 functions is false which I believe to be wrong. I
believe we have a few choices here, but no matter what we go with it
will be a bit of an overhaul. I will post a PR on bugzilla in a little
bit as this problem feels somewhat out of the scope of my patch now.

I think what I will do is instead of comparing the xobj parameter to
the DECL_CONTEXT of the xobj function, I will compare it to the type of
the iobj member function's object parameter. If I do it like this, it
will work as expected if we rewrite functions that are introduced with
a using declaration.

This might still cause problems, I will look into how the this pointer
for iobj member functions is determined again. Depending on how it is
determined, it might be possible to change the function signature of
iobj member functions without altering their behavior. It would be
incorrect, and would change the meaning of code, if changing the
function signature changed the type of the this pointer.

Anyhow, this is a fairly big change to consider so I won't pretend I
know what the right call is. But the way I've decided to implement
correspondence checking will be consistent with how GCC currently
(incorrectly) treats constraints on iobj member functions introduced
with a using declaration, so I think doing it this way is the right
choice for now.

Some days feel really unproductive when the majority is investigation
and thinking. This was one of them, but at least I'm confident that my
conclusions are correct. Aren't edge cases fun?

Alex

On Monday, January 1st, 2024 at 8:17 AM, waffl3x <waffl3x@protonmail.com> wrote:


> 
> 
> I've been at this for a while, and I'm not sure what the proper way to
> fix this is.
> 
> `struct S; struct B { void f(this S&) {} void g() {} }; struct S : B { using B::f; using B::g; void f() {} void g(this S&) {} }; int main() { S s{}; s.f(); s.g(); }`
> 
> In short, the call to f is ambiguous, but the call to g is not. I
> already know where the problem is, but since I share this code in
> places that don't know about whether a function was introduced by a
> using declaration (cand_parms_match), I don't want to rely on that to
> solve the problem.
> 
> `/* An iobj member function's object parameter can't be an unrelated type, if the xobj member function's object parameter is an unrelated type we know they must not correspond to each other. If the iobj member function was introduced with a using declaration, then the type of its object parameter is still that of the class we are currently adding a member function to, so this assumption holds true in that case as well. [over.match.funcs.general.4] For non-conversion functions that are implicit object member functions nominated by a using-declaration in a derived class, the function is considered to be a member of the derived class for the purpose of defining the type of the implicit object parameter. We don't get to bail yet out even if the xobj parameter is by-value as elaborated on below. This also implicitly handles xobj parameters of type pointer. */ if (DECL_CONTEXT (xobj_fn) != TYPE_MAIN_VARIANT (non_reference (xobj_param))) return false;`
> 
> I feel like what we are actually supposed to be doing to be to the
> letter of the standard is to be creating a new function entirely, with
> a decl_context of the original class, which sounds omega silly, and
> might bring a new set of problems.
> 
> I think I might have came up with an unfortunately fairly convoluted
> way to solve this just now, but I don't know if it brings another new
> set of problems. The assumptions I had when I originally implemented
> this in add_method bled through when I broke it out into it's own
> function. At the very least I need to better document how the function
> is intended to be used, at worst I'll need to consider whether it makes
> sense to be reusing this logic if the use cases are subtly different.
> 
> I don't think the latter is the case now though, I'm noticing GCC just
> has a bug in general with constraints and using declarations.
> 
> https://godbolt.org/z/EbGvjfG7E
> 
> So it might actually just be better to be rewriting functions that are
> introduced by using declarations, I have a feeling that will be what
> introduces the least pain.
> 
> I'm not sure where exactly GCC is deciding that a function introduced
> by a using declaration is different from an otherwise corresponding one
> declared directly in that class, but I have a feeling on where it is.
> Obviously it's in joust, but I'm not sure the object parameters are
> actually being compared.
> 
> I'll investigate this bug and get back to you, I imagine fixing it is
> going to be key to actually implementing the xobj case without hacks.
> 
> Finding both these issues has slowed down my next revision as I noticed
> the problem while cleaning up my implementation of CWG2789. I want to
> note, I am implementing it as if it specifies corresponding object
> arguments, not object arguments of the same type, as we previously
> discussed, I believe that to be the right resolution as there are
> really bad edge cases with the current wording.
> 
> Alex
> 
> On Tuesday, December 26th, 2023 at 9:37 AM, Jason Merrill jason@redhat.com wrote:
> 
> 
> 
> > On 12/23/23 02:10, waffl3x wrote:
> > 
> > > On Friday, December 22nd, 2023 at 10:26 AM, Jason Merrill jason@redhat.com wrote:
> > > 
> > > > On 12/22/23 04:01, waffl3x wrote:
> > > > 
> > > > > int n = 0;
> > > > > auto f = []<typename Self>(this Self){
> > > > > static_assert(__is_same (decltype(n), int));
> > > > > decltype((n)) a; // { dg-error {is not captured} }
> > > > > };
> > > > > f();
> > > > > 
> > > > > Could you clarify if this error being removed was intentional. I do
> > > > > recall that Patrick Palka wanted to remove this error in his patch, but
> > > > > it seemed to me like you stated it would be incorrect to allow it.
> > > > > Since the error is no longer present I assume I am misunderstanding the
> > > > > exchange.
> > > > > 
> > > > > In any case, let me know if I need to modify my test case or if this
> > > > > error needs to be added back in.
> > > > 
> > > > Removing the error was correct under
> > > > https://eel.is/c++draft/expr.prim#id.unqual-3
> > > > Naming n in that lambda would not refer a capture by copy, so the
> > > > decltype is the same as outside the lambda.
> > > 
> > > Alright, I've fixed my tests to reflect that.
> > > 
> > > I've got defaulting assignment operators working. Defaulting equality
> > > and comparison operators seemed to work out of the box somehow, so I
> > > just have to make some fleshed out tests for those cases.
> > > 
> > > There can always be more tests, I have a few ideas for what still needs
> > > to be covered, mostly with dependent lambdas. Tests for xobj conversion
> > > operators definitely need to be more fleshed out. I also need to
> > > formulate some tests to make sure constraints are not being taking into
> > > account when the object parameters should not correspond, but that's a
> > > little more tough to test for than the valid cases.
> > > 
> > > Other than tests though, is there anything you can think of that the
> > > patch is missing? Other than the aforementioned tests, I'm pretty
> > > confident everything is done.
> > > 
> > > To recap, I have CWG2789 implemented on my end with the change we
> > > discussed to require corresponding object parameters instead of the
> > > same type, and I have CWG2586 implemented. I can't recall what other
> > > outstanding issues we had, and my notes don't mention anything other
> > > than tests. So I'm assuming everything is good.
> > 
> > Sounds good! Did you mean to include the updated patch?
> > 
> > Jason

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113191

I've posted the report here, I ended up doing a bit more investigation,
so the contents might interest you. I'm really starting to think that
we want to do a more thorough re-engineering of how using declarations
are handled, instead of handling it with little hacks like the one
added to more_specialized_fn.

As I note in the report, the addition of xobj member functions really
makes [over.match.funcs.general.4] a lot more relevant, and I don't
think we can get away with not following it more closely anymore. I
know I'm wording myself here as if that passage has existed forever,
but I recognize it might be as recent as C++23 that it was added. I
don't mean to imply anything with how I'm wording it, it's just way
easier to express it this way. Especially since we really could get
away with these kinds of hacks if xobj member functions did not exist.
Unfortunately, the type of the implicit object parameter is suddenly
relevant for cases like this.

Anyway, as I've stated a few times, I'm going to implement my function
that checks correspondence of the object parameter of iobj and xobj
member functions assuming that iobj member functions introduced by
using declarations are handled properly. I think that's the best option
for my patch right now.

Well that investigation took the majority of my day. I'm just glad I'm
certain of what direction to take now.

Alex


On Monday, January 1st, 2024 at 8:34 AM, waffl3x <waffl3x@protonmail.com> wrote:


> 
> 
> That was faster than I expected, the problem is exactly just that we
> aren't implementing [over.match.funcs.general.4] at all. The result of
> compparms for the 2 functions is false which I believe to be wrong. I
> believe we have a few choices here, but no matter what we go with it
> will be a bit of an overhaul. I will post a PR on bugzilla in a little
> bit as this problem feels somewhat out of the scope of my patch now.
> 
> I think what I will do is instead of comparing the xobj parameter to
> the DECL_CONTEXT of the xobj function, I will compare it to the type of
> the iobj member function's object parameter. If I do it like this, it
> will work as expected if we rewrite functions that are introduced with
> a using declaration.
> 
> This might still cause problems, I will look into how the this pointer
> for iobj member functions is determined again. Depending on how it is
> determined, it might be possible to change the function signature of
> iobj member functions without altering their behavior. It would be
> incorrect, and would change the meaning of code, if changing the
> function signature changed the type of the this pointer.
> 
> Anyhow, this is a fairly big change to consider so I won't pretend I
> know what the right call is. But the way I've decided to implement
> correspondence checking will be consistent with how GCC currently
> (incorrectly) treats constraints on iobj member functions introduced
> with a using declaration, so I think doing it this way is the right
> choice for now.
> 
> Some days feel really unproductive when the majority is investigation
> and thinking. This was one of them, but at least I'm confident that my
> conclusions are correct. Aren't edge cases fun?
> 
> Alex
> 
> On Monday, January 1st, 2024 at 8:17 AM, waffl3x waffl3x@protonmail.com wrote:
> 
> 
> 
> > I've been at this for a while, and I'm not sure what the proper way to
> > fix this is.
> > 
> > `struct S; struct B { void f(this S&) {} void g() {} }; struct S : B { using B::f; using B::g; void f() {} void g(this S&) {} }; int main() { S s{}; s.f(); s.g(); }`
> > 
> > In short, the call to f is ambiguous, but the call to g is not. I
> > already know where the problem is, but since I share this code in
> > places that don't know about whether a function was introduced by a
> > using declaration (cand_parms_match), I don't want to rely on that to
> > solve the problem.
> > 
> > `/* An iobj member function's object parameter can't be an unrelated type, if the xobj member function's object parameter is an unrelated type we know they must not correspond to each other. If the iobj member function was introduced with a using declaration, then the type of its object parameter is still that of the class we are currently adding a member function to, so this assumption holds true in that case as well. [over.match.funcs.general.4] For non-conversion functions that are implicit object member functions nominated by a using-declaration in a derived class, the function is considered to be a member of the derived class for the purpose of defining the type of the implicit object parameter. We don't get to bail yet out even if the xobj parameter is by-value as elaborated on below. This also implicitly handles xobj parameters of type pointer. */ if (DECL_CONTEXT (xobj_fn) != TYPE_MAIN_VARIANT (non_reference (xobj_param))) return false;`
> > 
> > I feel like what we are actually supposed to be doing to be to the
> > letter of the standard is to be creating a new function entirely, with
> > a decl_context of the original class, which sounds omega silly, and
> > might bring a new set of problems.
> > 
> > I think I might have came up with an unfortunately fairly convoluted
> > way to solve this just now, but I don't know if it brings another new
> > set of problems. The assumptions I had when I originally implemented
> > this in add_method bled through when I broke it out into it's own
> > function. At the very least I need to better document how the function
> > is intended to be used, at worst I'll need to consider whether it makes
> > sense to be reusing this logic if the use cases are subtly different.
> > 
> > I don't think the latter is the case now though, I'm noticing GCC just
> > has a bug in general with constraints and using declarations.
> > 
> > https://godbolt.org/z/EbGvjfG7E
> > 
> > So it might actually just be better to be rewriting functions that are
> > introduced by using declarations, I have a feeling that will be what
> > introduces the least pain.
> > 
> > I'm not sure where exactly GCC is deciding that a function introduced
> > by a using declaration is different from an otherwise corresponding one
> > declared directly in that class, but I have a feeling on where it is.
> > Obviously it's in joust, but I'm not sure the object parameters are
> > actually being compared.
> > 
> > I'll investigate this bug and get back to you, I imagine fixing it is
> > going to be key to actually implementing the xobj case without hacks.
> > 
> > Finding both these issues has slowed down my next revision as I noticed
> > the problem while cleaning up my implementation of CWG2789. I want to
> > note, I am implementing it as if it specifies corresponding object
> > arguments, not object arguments of the same type, as we previously
> > discussed, I believe that to be the right resolution as there are
> > really bad edge cases with the current wording.
> > 
> > Alex
> > 
> > On Tuesday, December 26th, 2023 at 9:37 AM, Jason Merrill jason@redhat.com wrote:
> > 
> > > On 12/23/23 02:10, waffl3x wrote:
> > > 
> > > > On Friday, December 22nd, 2023 at 10:26 AM, Jason Merrill jason@redhat.com wrote:
> > > > 
> > > > > On 12/22/23 04:01, waffl3x wrote:
> > > > > 
> > > > > > int n = 0;
> > > > > > auto f = []<typename Self>(this Self){
> > > > > > static_assert(__is_same (decltype(n), int));
> > > > > > decltype((n)) a; // { dg-error {is not captured} }
> > > > > > };
> > > > > > f();
> > > > > > 
> > > > > > Could you clarify if this error being removed was intentional. I do
> > > > > > recall that Patrick Palka wanted to remove this error in his patch, but
> > > > > > it seemed to me like you stated it would be incorrect to allow it.
> > > > > > Since the error is no longer present I assume I am misunderstanding the
> > > > > > exchange.
> > > > > > 
> > > > > > In any case, let me know if I need to modify my test case or if this
> > > > > > error needs to be added back in.
> > > > > 
> > > > > Removing the error was correct under
> > > > > https://eel.is/c++draft/expr.prim#id.unqual-3
> > > > > Naming n in that lambda would not refer a capture by copy, so the
> > > > > decltype is the same as outside the lambda.
> > > > 
> > > > Alright, I've fixed my tests to reflect that.
> > > > 
> > > > I've got defaulting assignment operators working. Defaulting equality
> > > > and comparison operators seemed to work out of the box somehow, so I
> > > > just have to make some fleshed out tests for those cases.
> > > > 
> > > > There can always be more tests, I have a few ideas for what still needs
> > > > to be covered, mostly with dependent lambdas. Tests for xobj conversion
> > > > operators definitely need to be more fleshed out. I also need to
> > > > formulate some tests to make sure constraints are not being taking into
> > > > account when the object parameters should not correspond, but that's a
> > > > little more tough to test for than the valid cases.
> > > > 
> > > > Other than tests though, is there anything you can think of that the
> > > > patch is missing? Other than the aforementioned tests, I'm pretty
> > > > confident everything is done.
> > > > 
> > > > To recap, I have CWG2789 implemented on my end with the change we
> > > > discussed to require corresponding object parameters instead of the
> > > > same type, and I have CWG2586 implemented. I can't recall what other
> > > > outstanding issues we had, and my notes don't mention anything other
> > > > than tests. So I'm assuming everything is good.
> > > 
> > > Sounds good! Did you mean to include the updated patch?
> > > 
> > > Jason

I found a bugged test while finishing off the patch.
gcc/testsuite/g++.dg/cpp0x/lambda/lambda-type.C
line 72 has
same_type<decltype((i)),int const&>(); // { dg-error "" "not captured" }
The behavior has changed now, and this test should have failed, but
didn't as it has an empty regex expression. The actual error being
emitted here is this:
error: invalid use of incomplete type 'struct same_type<int&, const int&>'

This still happens to pass because the types don't match, but it should
be fixed to reflect the new intended behavior.

The finished patch will be coming soon(ish), I had to make some changes
so you will have to look over it quickly but hopefully it will be split
up enough to make that convenient.

Alex


On Monday, January 1st, 2024 at 10:12 AM, waffl3x <waffl3x@protonmail.com> wrote:


> 
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113191
> 
> I've posted the report here, I ended up doing a bit more investigation,
> so the contents might interest you. I'm really starting to think that
> we want to do a more thorough re-engineering of how using declarations
> are handled, instead of handling it with little hacks like the one
> added to more_specialized_fn.
> 
> As I note in the report, the addition of xobj member functions really
> makes [over.match.funcs.general.4] a lot more relevant, and I don't
> think we can get away with not following it more closely anymore. I
> know I'm wording myself here as if that passage has existed forever,
> but I recognize it might be as recent as C++23 that it was added. I
> don't mean to imply anything with how I'm wording it, it's just way
> easier to express it this way. Especially since we really could get
> away with these kinds of hacks if xobj member functions did not exist.
> Unfortunately, the type of the implicit object parameter is suddenly
> relevant for cases like this.
> 
> Anyway, as I've stated a few times, I'm going to implement my function
> that checks correspondence of the object parameter of iobj and xobj
> member functions assuming that iobj member functions introduced by
> using declarations are handled properly. I think that's the best option
> for my patch right now.
> 
> Well that investigation took the majority of my day. I'm just glad I'm
> certain of what direction to take now.
> 
> Alex
> 
> 
> On Monday, January 1st, 2024 at 8:34 AM, waffl3x waffl3x@protonmail.com wrote:
> 
> 
> 
> > That was faster than I expected, the problem is exactly just that we
> > aren't implementing [over.match.funcs.general.4] at all. The result of
> > compparms for the 2 functions is false which I believe to be wrong. I
> > believe we have a few choices here, but no matter what we go with it
> > will be a bit of an overhaul. I will post a PR on bugzilla in a little
> > bit as this problem feels somewhat out of the scope of my patch now.
> > 
> > I think what I will do is instead of comparing the xobj parameter to
> > the DECL_CONTEXT of the xobj function, I will compare it to the type of
> > the iobj member function's object parameter. If I do it like this, it
> > will work as expected if we rewrite functions that are introduced with
> > a using declaration.
> > 
> > This might still cause problems, I will look into how the this pointer
> > for iobj member functions is determined again. Depending on how it is
> > determined, it might be possible to change the function signature of
> > iobj member functions without altering their behavior. It would be
> > incorrect, and would change the meaning of code, if changing the
> > function signature changed the type of the this pointer.
> > 
> > Anyhow, this is a fairly big change to consider so I won't pretend I
> > know what the right call is. But the way I've decided to implement
> > correspondence checking will be consistent with how GCC currently
> > (incorrectly) treats constraints on iobj member functions introduced
> > with a using declaration, so I think doing it this way is the right
> > choice for now.
> > 
> > Some days feel really unproductive when the majority is investigation
> > and thinking. This was one of them, but at least I'm confident that my
> > conclusions are correct. Aren't edge cases fun?
> > 
> > Alex
> > 
> > On Monday, January 1st, 2024 at 8:17 AM, waffl3x waffl3x@protonmail.com wrote:
> > 
> > > I've been at this for a while, and I'm not sure what the proper way to
> > > fix this is.
> > > 
> > > `struct S; struct B { void f(this S&) {} void g() {} }; struct S : B { using B::f; using B::g; void f() {} void g(this S&) {} }; int main() { S s{}; s.f(); s.g(); }`
> > > 
> > > In short, the call to f is ambiguous, but the call to g is not. I
> > > already know where the problem is, but since I share this code in
> > > places that don't know about whether a function was introduced by a
> > > using declaration (cand_parms_match), I don't want to rely on that to
> > > solve the problem.
> > > 
> > > `/* An iobj member function's object parameter can't be an unrelated type, if the xobj member function's object parameter is an unrelated type we know they must not correspond to each other. If the iobj member function was introduced with a using declaration, then the type of its object parameter is still that of the class we are currently adding a member function to, so this assumption holds true in that case as well. [over.match.funcs.general.4] For non-conversion functions that are implicit object member functions nominated by a using-declaration in a derived class, the function is considered to be a member of the derived class for the purpose of defining the type of the implicit object parameter. We don't get to bail yet out even if the xobj parameter is by-value as elaborated on below. This also implicitly handles xobj parameters of type pointer. */ if (DECL_CONTEXT (xobj_fn) != TYPE_MAIN_VARIANT (non_reference (xobj_param))) return false;`
> > > 
> > > I feel like what we are actually supposed to be doing to be to the
> > > letter of the standard is to be creating a new function entirely, with
> > > a decl_context of the original class, which sounds omega silly, and
> > > might bring a new set of problems.
> > > 
> > > I think I might have came up with an unfortunately fairly convoluted
> > > way to solve this just now, but I don't know if it brings another new
> > > set of problems. The assumptions I had when I originally implemented
> > > this in add_method bled through when I broke it out into it's own
> > > function. At the very least I need to better document how the function
> > > is intended to be used, at worst I'll need to consider whether it makes
> > > sense to be reusing this logic if the use cases are subtly different.
> > > 
> > > I don't think the latter is the case now though, I'm noticing GCC just
> > > has a bug in general with constraints and using declarations.
> > > 
> > > https://godbolt.org/z/EbGvjfG7E
> > > 
> > > So it might actually just be better to be rewriting functions that are
> > > introduced by using declarations, I have a feeling that will be what
> > > introduces the least pain.
> > > 
> > > I'm not sure where exactly GCC is deciding that a function introduced
> > > by a using declaration is different from an otherwise corresponding one
> > > declared directly in that class, but I have a feeling on where it is.
> > > Obviously it's in joust, but I'm not sure the object parameters are
> > > actually being compared.
> > > 
> > > I'll investigate this bug and get back to you, I imagine fixing it is
> > > going to be key to actually implementing the xobj case without hacks.
> > > 
> > > Finding both these issues has slowed down my next revision as I noticed
> > > the problem while cleaning up my implementation of CWG2789. I want to
> > > note, I am implementing it as if it specifies corresponding object
> > > arguments, not object arguments of the same type, as we previously
> > > discussed, I believe that to be the right resolution as there are
> > > really bad edge cases with the current wording.
> > > 
> > > Alex
> > > 
> > > On Tuesday, December 26th, 2023 at 9:37 AM, Jason Merrill jason@redhat.com wrote:
> > > 
> > > > On 12/23/23 02:10, waffl3x wrote:
> > > > 
> > > > > On Friday, December 22nd, 2023 at 10:26 AM, Jason Merrill jason@redhat.com wrote:
> > > > > 
> > > > > > On 12/22/23 04:01, waffl3x wrote:
> > > > > > 
> > > > > > > int n = 0;
> > > > > > > auto f = []<typename Self>(this Self){
> > > > > > > static_assert(__is_same (decltype(n), int));
> > > > > > > decltype((n)) a; // { dg-error {is not captured} }
> > > > > > > };
> > > > > > > f();
> > > > > > > 
> > > > > > > Could you clarify if this error being removed was intentional. I do
> > > > > > > recall that Patrick Palka wanted to remove this error in his patch, but
> > > > > > > it seemed to me like you stated it would be incorrect to allow it.
> > > > > > > Since the error is no longer present I assume I am misunderstanding the
> > > > > > > exchange.
> > > > > > > 
> > > > > > > In any case, let me know if I need to modify my test case or if this
> > > > > > > error needs to be added back in.
> > > > > > 
> > > > > > Removing the error was correct under
> > > > > > https://eel.is/c++draft/expr.prim#id.unqual-3
> > > > > > Naming n in that lambda would not refer a capture by copy, so the
> > > > > > decltype is the same as outside the lambda.
> > > > > 
> > > > > Alright, I've fixed my tests to reflect that.
> > > > > 
> > > > > I've got defaulting assignment operators working. Defaulting equality
> > > > > and comparison operators seemed to work out of the box somehow, so I
> > > > > just have to make some fleshed out tests for those cases.
> > > > > 
> > > > > There can always be more tests, I have a few ideas for what still needs
> > > > > to be covered, mostly with dependent lambdas. Tests for xobj conversion
> > > > > operators definitely need to be more fleshed out. I also need to
> > > > > formulate some tests to make sure constraints are not being taking into
> > > > > account when the object parameters should not correspond, but that's a
> > > > > little more tough to test for than the valid cases.
> > > > > 
> > > > > Other than tests though, is there anything you can think of that the
> > > > > patch is missing? Other than the aforementioned tests, I'm pretty
> > > > > confident everything is done.
> > > > > 
> > > > > To recap, I have CWG2789 implemented on my end with the change we
> > > > > discussed to require corresponding object parameters instead of the
> > > > > same type, and I have CWG2586 implemented. I can't recall what other
> > > > > outstanding issues we had, and my notes don't mention anything other
> > > > > than tests. So I'm assuming everything is good.
> > > > 
> > > > Sounds good! Did you mean to include the updated patch?
> > > > 
> > > > Jason