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

[Ken Matsui <kmatsui@cs.washington.edu>]

On Mon, Oct 23, 2023 at 2:23 PM Jason Merrill <jason@redhat.com> wrote:
>
> 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?
>

Thank you so much for your review!  I will apply your suggestions.

> Jason
>