On Mon, Oct 23, 2023 at 2:23 PM Jason Merrill <ja...@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
>
