Re: [PATCH] c++: Implement C++23 P1169R4 - static operator() [PR106651]

[Jason Merrill <jason@redhat.com>]

On 9/17/22 02:42, Jakub Jelinek wrote:
> On Sat, Sep 17, 2022 at 01:23:59AM +0200, Jason Merrill wrote:
>> On 9/13/22 12:42, Jakub Jelinek wrote:
>>> The following patch attempts to implement C++23 P1169R4 - static operator()
>>> paper's compiler side (there is some small library side too not implemented
>>> yet).  This allows static members as user operator() declarations and
>>> static specifier on lambdas without lambda capture.  As decl specifier
>>> parsing doesn't track about the presence and locations of all specifiers,
>>> the patch unfortunately replaces the diagnostics about duplicate mutable
>>> with diagnostics about conflicting specifiers because the information
>>> whether it was mutable mutable, mutable static, static mutable or static
>>> static is lost.
>>
>> I wonder why we don't give an error when setting the
>> conflicting_specifiers_p flag in cp_parser_set_storage_class?  We should be
>> able to give a better diagnostic at that point.
> 
> I will try that.
> 
>>> Beyond this, the synthetized conversion operator changes
>>> for static lambdas as it can just return the operator() static method
>>> address, doesn't need to create a thunk for it.
>>> The change I'm least sure about is the call.cc (joust) change, one thing
>>> is that we ICEd because we assumed that len could be different only if
>>> both candidates are direct calls but it can be one direct and one indirect
>>> call,
>>
>> How do you mean?
> 
> That is either on the
> struct less {
>      static constexpr auto operator()(int i, int j) -> bool {
>          return i < j;
>      }
> 
>      using P = bool(*)(int, int);
>      operator P() const { return operator(); }
> };
> 
> static_assert(less{}(1, 2));
> testcase from the paper (S in static-operator-call1.C) or any static lambda
> which also has static operator() and cast operator.
> The paper then talks about
> operator()(contrived-parameter, int, int);
> call-function(bool(*)(int, int), int, int);
> which is exactly what caused the ICE, one of the cand?->fn was the static
> operator(), the other cand?->fn isn't a FUNCTION_DECL, but a function
> pointer (what the cast operator returns).

Ah, makes sense.

>>> +	{
>>> +	  /* C++23 [over.best.ics.general] says:
>>> +	     When the parameter is the implicit object parameter of a static
>>> +	     member function, the implicit conversion sequence is a standard
>>> +	     conversion sequence that is neither better nor worse than any
>>> +	     other standard conversion sequence.
>>> +	     Apply this for C++23 or when the static member function is
>>> +	     overloaded call operator (C++23 feature we accept as an
>>> +	     extension).  */
>>> +	  if ((cxx_dialect >= cxx23
>>> +	       || (DECL_OVERLOADED_OPERATOR_P (cand1->fn)
>>> +		   && DECL_OVERLOADED_OPERATOR_IS (cand1->fn, CALL_EXPR)))
>>> +	      && CONVERSION_RANK (cand2->convs[0]) >= cr_user)
>>> +	    winner = -1;
>>
>> I don't know what you're trying to do here.  The above passage describes how
>> we've always compared static to non-static, which should work the same way
>> for operator().
> 
> It doesn't work for the static operator() case vs. pointer returned by
> cast operator, with just the
> -      int static_1 = DECL_STATIC_FUNCTION_P (cand1->fn);
> -      int static_2 = DECL_STATIC_FUNCTION_P (cand2->fn);
> +      int static_1 = (TREE_CODE (cand1->fn) == FUNCTION_DECL
> +                     && DECL_STATIC_FUNCTION_P (cand1->fn));
> +      int static_2 = (TREE_CODE (cand2->fn) == FUNCTION_DECL
> +                     && DECL_STATIC_FUNCTION_P (cand2->fn));
> 
> -      if (DECL_CONSTRUCTOR_P (cand1->fn)
> +      if (TREE_CODE (cand1->fn) == FUNCTION_DECL
> +         && TREE_CODE (cand2->fn) == FUNCTION_DECL
> +         && DECL_CONSTRUCTOR_P (cand1->fn)
> changes in joust so that it doesn't ICE, the call is ambiguous.
> Previously the standard had:
> "If F is a static member function, ICS1(F) is defined such that ICS1(F) is neither better
> nor worse than ICS1(G) for any function G, and, symmetrically, ICS1(G) is neither better
> nor worse than ICS1(F); otherwise,"
> but that was removed and instead:
> "When the parameter is the implicit object parameter of a static member function, the
> implicit conversion sequence is a standard conversion sequence that is neither better
> nor worse than any other standard conversion sequence."
> was added elsewhere.  The code implements the former.  We don't have any
> conversion for the non-existing object parameter to static, so what I want
> to do is keep doing what we were before if the other candidate's conversion
> on the first parameter is standard (CONVERSION_RANK (cand2->convs[0]) < cr_user)
> and otherwise indicate that the static operator() has there a standard
> conversion and so it should be better than any user/ellipsis/bad conversion.
> Now, it could be done just for cxx_dialect >= cxx23 as that is pedantically
> what the standard says, but if we allow with a pedwarn static operator()
> in earlier standards, I think we better treat it there the same, because
> otherwise one can't call any static lambdas (all those calls would be
> ambiguous).

Ah, OK.  I don't think you need to check for C++23 or CALL_EXPR at all, 
just CONVERSION_RANK of the other candidate conversion.

And I notice that you have winner = -1 in both cases; the first should 
be winner = 1.

>>> @@ -1135,9 +1141,13 @@ maybe_add_lambda_conv_op (tree type)
>>>      tree fn_args = NULL_TREE;
>>>      {
>>>        int ix = 0;
>>> -    tree src = DECL_CHAIN (DECL_ARGUMENTS (callop));
>>> +    tree src = DECL_ARGUMENTS (callop);
>>>        tree tgt = NULL;
>>> +    if (thisarg)
>>> +      src = DECL_CHAIN (src);
>>
>> You could use FUNCTION_FIRST_USER_PARM to skip 'this' if present.
> 
> Ok, will try.
> 
>>> +    else if (!decltype_call)
>>> +      src = NULL_TREE;
>>>        while (src)
>>>          {
>>>    	tree new_node = copy_node (src);
>>> @@ -1160,12 +1170,15 @@ maybe_add_lambda_conv_op (tree type)
>>>    	if (generic_lambda_p)
>>>    	  {
>>>    	    tree a = tgt;
>>> -	    if (DECL_PACK_P (tgt))
>>> +	    if (thisarg)
>>
>> This is wrong, pack expansion handling has nothing to do with 'this'. Only
>> the assignment to CALL_EXPR_ARG should depend on thisarg.
> 
> The point was that make_pack_expansion etc. is only used for what
> is later then stored to CALL_EXPR_ARG.  As we don't need to construct
> call at all for the static lambda case, that looked like wasted effort.

Ah, you're right, I thought it was also used for decltype_call.

>>> @@ -1203,14 +1216,18 @@ maybe_add_lambda_conv_op (tree type)
>>>    			   direct_argvec->address ());
>>>        }
>>> -  CALL_FROM_THUNK_P (call) = 1;
>>> -  SET_EXPR_LOCATION (call, UNKNOWN_LOCATION);
>>> +  if (thisarg)
>>> +    {
>>> +      CALL_FROM_THUNK_P (call) = 1;
>>> +      SET_EXPR_LOCATION (call, UNKNOWN_LOCATION);
>>> +    }
>>> -  tree stattype = build_function_type (fn_result, FUNCTION_ARG_CHAIN (callop));
>>> -  stattype = (cp_build_type_attribute_variant
>>> -	      (stattype, TYPE_ATTRIBUTES (optype)));
>>> -  if (flag_noexcept_type
>>> -      && TYPE_NOTHROW_P (TREE_TYPE (callop)))
>>> +  tree stattype
>>> +    = build_function_type (fn_result, thisarg ? FUNCTION_ARG_CHAIN (callop)
>>> +				      : TYPE_ARG_TYPES (optype));
>>> +  stattype = cp_build_type_attribute_variant (stattype,
>>> +					      TYPE_ATTRIBUTES (optype));
>>> +  if (flag_noexcept_type && TYPE_NOTHROW_P (TREE_TYPE (callop)))
>>>        stattype = build_exception_variant (stattype, noexcept_true_spec);
>>
>> If !thisarg, shouldn't stattype just be optype?  No need to rebuild it in
>> that case.
> 
> I thought it should be auto-deduced if it isn't yet.

Ah, yes, of course.

You can use FUNCTION_FIRST_USER_PARMTYPE instead of the ?:.

The reformatting of the next two statements seems unnecessary.

> If it doesn't need to
> be, sure, we wouldn't need to bother with decltype_call for !thisarg
> either and just not do the loop on arguments etc.
> I've tried to create a testcase where I could see the generic static lambda
> but I failed to construct one, even with the template <...> lambda syntax
> I can't use a template id of the lambda to instantiate some particular case
> and then use *.  Would passing a generic lambda to a function with
> some function pointer argument trigger it?

How about

void f()
{
   auto l = [] (auto x) static { return x; };
   int (*p)(int) = l;
}

?

Jason