Re: [PATCH v23 32/33] c++: Implement __is_invocable built-in trait

[Jason Merrill <jason@redhat.com>]

On 10/20/23 17:37, Patrick Palka wrote:
> On Fri, 20 Oct 2023, Patrick Palka wrote:
> 
>> On Fri, 20 Oct 2023, Patrick Palka wrote:
>>
>>> On Fri, 20 Oct 2023, Ken Matsui wrote:
>>>
>>>> This patch implements built-in trait for std::is_invocable.
>>>
>>> Nice!  My email client unfortunately ate my first review attempt, so
>>> apologies for my brevity this time around.
>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> 	* cp-trait.def: Define __is_invocable.
>>>> 	* constraint.cc (diagnose_trait_expr): Handle CPTK_IS_INVOCABLE.
>>>> 	* semantics.cc (trait_expr_value): Likewise.
>>>> 	(finish_trait_expr): Likewise.
>>>> 	(is_invocable_p): New function.
>>>> 	* method.h: New file to export build_trait_object in method.cc.

Given how much larger semantics.cc is than method.cc, maybe let's put 
is_invocable_p in method.cc instead?  And in general declarations can go 
in cp-tree.h.

>>>> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
>>>> index 7cccbae5287..cc2e400531a 100644
>>>> --- a/gcc/cp/semantics.cc
>>>> +++ b/gcc/cp/semantics.cc
>>>> @@ -45,6 +45,10 @@ along with GCC; see the file COPYING3.  If not see
>>>>   #include "gomp-constants.h"
>>>>   #include "predict.h"
>>>>   #include "memmodel.h"
>>>> +#include "method.h"
>>>> +
>>>> +#include "print-tree.h"
>>>> +#include "tree-pretty-print.h"
>>>>   
>>>>   /* There routines provide a modular interface to perform many parsing
>>>>      operations.  They may therefore be used during actual parsing, or
>>>> @@ -11714,6 +11718,133 @@ classtype_has_nothrow_assign_or_copy_p (tree type, bool assign_p)
>>>>     return saw_copy;
>>>>   }
>>>>   
>>>> +/* Return true if FN_TYPE is invocable with the given ARG_TYPES.  */
>>>> +
>>>> +static bool
>>>> +is_invocable_p (tree fn_type, tree arg_types)
> 
> (Sorry for the spam)  We'll eventually want to implement a built-in for
> invoke_result, so perhaps we should preemptively factor out the bulk
> of this function into a 'build_INVOKE' helper function that returns the
> built tree?
> 
>>>> +{
>>>> +  /* ARG_TYPES must be a TREE_VEC.  */
>>>> +  gcc_assert (TREE_CODE (arg_types) == TREE_VEC);
>>>> +
>>>> +  /* Access check is required to determine if the given is invocable.  */
>>>> +  deferring_access_check_sentinel acs (dk_no_deferred);
>>>> +
>>>> +  /* std::is_invocable is an unevaluated context.  */
>>>> +  cp_unevaluated cp_uneval_guard;
>>>> +
>>>> +  bool is_ptrdatamem;
>>>> +  bool is_ptrmemfunc;
>>>> +  if (TREE_CODE (fn_type) == REFERENCE_TYPE)
>>>> +    {
>>>> +      tree deref_fn_type = TREE_TYPE (fn_type);
>>>> +      is_ptrdatamem = TYPE_PTRDATAMEM_P (deref_fn_type);
>>>> +      is_ptrmemfunc = TYPE_PTRMEMFUNC_P (deref_fn_type);
>>>> +
>>>> +      /* Dereference fn_type if it is a pointer to member.  */
>>>> +      if (is_ptrdatamem || is_ptrmemfunc)
>>>> +	fn_type = deref_fn_type;
>>>> +    }
>>>> +  else
>>>> +    {
>>>> +      is_ptrdatamem = TYPE_PTRDATAMEM_P (fn_type);
>>>> +      is_ptrmemfunc = TYPE_PTRMEMFUNC_P (fn_type);
>>>> +    }
>>>> +
>>>> +  if (is_ptrdatamem && TREE_VEC_LENGTH (arg_types) != 1)
>>>> +    /* A pointer to data member with non-one argument is not invocable.  */
>>>> +    return false;
>>>> +
>>>> +  if (is_ptrmemfunc && TREE_VEC_LENGTH (arg_types) == 0)
>>>> +    /* A pointer to member function with no arguments is not invocable.  */
>>>> +    return false;
>>>> +
>>>> +  /* Construct an expression of a pointer to member.  */
>>>> +  tree datum;
>>>> +  if (is_ptrdatamem || is_ptrmemfunc)
>>>> +    {
>>>> +      tree datum_type = TREE_VEC_ELT (arg_types, 0);
>>>> +
>>>> +      /* Dereference datum.  */
>>>> +      if (CLASS_TYPE_P (datum_type))
>>>> +	{
>>>> +	  bool is_refwrap = false;
>>>> +
>>>> +	  tree datum_decl = TYPE_NAME (TYPE_MAIN_VARIANT (datum_type));
>>>> +	  if (decl_in_std_namespace_p (datum_decl))
>>>> +	    {
>>>> +	      tree name = DECL_NAME (datum_decl);
>>>> +	      if (name && (id_equal (name, "reference_wrapper")))
>>>> +		{
>>>> +		  /* Handle std::reference_wrapper.  */
>>>> +		  is_refwrap = true;
>>>> +		  datum_type = cp_build_reference_type (datum_type, false);

Why do you change datum_type from std::reference_wrapper<...> to 
std::reference_wrapper<...>&?

>>>> +		}
>>>> +	    }
>>>> +
>>>> +	  datum = build_trait_object (datum_type);
>>>> +
>>>> +	  /* If datum_type was not std::reference_wrapper, check if it has
>>>> +	     operator*() overload.  If datum_type was std::reference_wrapper,
>>>> +	     avoid dereferencing the datum twice.  */
>>>> +	  if (!is_refwrap)
>>>> +	    if (get_class_binding (datum_type, get_identifier ("operator*")))
>>>
>>> We probably should use lookup_member instead of get_class_binding since
>>> IIUC the latter doesn't look into bases:
>>>
>>>    struct A { int m; };
>>>    struct B { A& operator*(): };
>>>    struct C : B { };
>>>    static_assert(std::is_invocable_v<int A::*, C>);
>>>
>>> However, I notice that the specification of INVOKE
>>> (https://eel.is/c++draft/func.require#lib:INVOKE) doesn't mention name
>>> lookup at all so it strikes me as suspicious that we'd perform name
>>> lookup here.

Agreed.  It seems that whether or not to build_x_indirect_ref should 
depend instead on whether f is a pointer to a member of decltype(t1) (as 
well as is_refwrap).

>>>  I think this would misbehave for:
>>>
>>>    struct A { };
>>>    struct B : A { A& operator*() = delete; };
>>>    static_assert(std::is_invocable_v<int A::*, B>);
>>>
>>>    struct C : private A { A& operator*(); };
>>>    static_assert(std::is_invocable_v<int A::*, C>);
>>
>> Oops, this static_assert is missing a !
>>
>>>
>>> ultimately because we end up choosing the dereference form of INVOKE,
>>> but according to 1.1/1.4 we should choose the non-dereference form?
>>>
>>>> +	      /* Handle operator*().  */
>>>> +	      datum = build_x_indirect_ref (UNKNOWN_LOCATION, datum,
>>>> +					    RO_UNARY_STAR, NULL_TREE,
>>>> +					    tf_none);
>>>> +	}
>>>> +      else if (POINTER_TYPE_P (datum_type))
>>>> +	datum = build_trait_object (TREE_TYPE (datum_type));
>>>> +      else
>>>> +	datum = build_trait_object (datum_type);
>>>> +    }
>>>> +
>>>> +  /* Build a function expression.  */
>>>> +  tree fn;
>>>> +  if (is_ptrdatamem)
>>>> +    fn = build_m_component_ref (datum, build_trait_object (fn_type), tf_none);
>>>
>>> Maybe exit early for the is_ptrdatamem case here (and simplify the rest
>>> of the function accordingly)?
>>>
>>>> +  else if (is_ptrmemfunc)
>>>> +    fn = build_trait_object (TYPE_PTRMEMFUNC_FN_TYPE (fn_type));

Why not use build_m_component_ref and build_offset_ref_call_from_tree 
like it would if you wrote (t1.*f)() directly?

Jason