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

[Jason Merrill <jason@redhat.com>]

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