Re: [PATCH v2] Allow calling of user-defined function call operators

[Guinevere Larsen <blarsen@redhat.com>]

On 5/3/24 15:51, Hannes Domani wrote:
>   Am Freitag, 3. Mai 2024 um 20:29:47 MESZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:
>
>> On 4/27/24 13:36, Hannes Domani wrote:
>>> Currently it's not possible to call user-defined function call
>>> operators, at least not without specifying operator() directly:
>>> ```
>>> (gdb) l 1
>>> 1      struct S {
>>> 2        int operator() (int x) { return x + 5; }
>>> 3      };
>>> 4
>>> 5      int main () {
>>> 6        S s;
>>> 7
>>> 8        return s(23);
>>> 9      }
>>> (gdb) p s(10)
>>> Invalid data type for function to be called.
>>> (gdb) p s.operator()(10)
>>> $1 = 15
>>> ```
>>>
>>> This now looks if an user-defined call operator is available when
>>> trying to 'call' a struct value, and calls it instead, making this
>>> possible:
>>> ```
>>> (gdb) p s(10)
>>> $1 = 15
>>> ```
>>>
>>> The change in operation::evaluate_funcall is to make sure the type
>>> fields are only used for function types, only they use them as the
>>> argument types.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12213
>>> ---
>>> v2:
>>> - Move the logic into evaluate_subexp_do_call, to avoid duplication in
>>>      every evaluate_funcall of each operation subclass.
>>>      This makes it now work for some cases it didn't in v1, like if it's
>>>      called on a class member (`print c.m(5)` in the new test).
>>> - Added tests for other (struct member) operations.
>>> ---
>>>    gdb/eval.c                      | 29 ++++++++++++++++++++++++++---
>>>    gdb/testsuite/gdb.cp/userdef.cc  | 22 ++++++++++++++++++++++
>>>    gdb/testsuite/gdb.cp/userdef.exp |  7 +++++++
>>>    3 files changed, 55 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/gdb/eval.c b/gdb/eval.c
>>> index 6b752e70635..8d5c354f480 100644
>>> --- a/gdb/eval.c
>>> +++ b/gdb/eval.c
>>> @@ -588,14 +588,35 @@ evaluate_subexp_do_call (expression *exp, enum noside noside,
>>>    {
>>>      if (callee == NULL)
>>>        error (_("Cannot evaluate function -- may be inlined"));
>>> +
>>> +  type *ftype = callee->type ();
>>> +
>>> +  /* If the callee is a struct, there might be a user-defined function call
>>> +    operator that should be used instead.  */
>>> +  std::vector<value *> vals;
>>> +  if (overload_resolution
>>> +      && exp->language_defn->la_language == language_cplus
>>> +      && check_typedef (ftype)->code () == TYPE_CODE_STRUCT)
>>> +    {
>>> +      vals.resize (argvec.size () + 1);
>>> +
>>> +      vals[0] = value_addr (callee);
>>> +      for (int i = 0; i < argvec.size (); ++i)
>>> +    vals[i + 1] = argvec[i];
>>> +
>>> +      int static_memfuncp;
>>> +      find_overload_match (vals, "operator()", METHOD, &vals[0], nullptr,
>>> +              &callee, nullptr, &static_memfuncp, 0, noside);
>>> +      if (!static_memfuncp)
>>> +    argvec = vals;
>> I don't really understand this change. From what I can see in this
>> patch, you're just shifting all values in argvec one to the right, to
>> add the callee address. Wouldn't this necessitate a change in the logic
>> for the rest of the function?
> Yes, this adds the 'this' pointer as the first argument for operator().
Ah, thanks for explaining! I think a comment would be pretty helpful in 
this situation.
> I'm not sure why this would change the logic for the rest of the function.

My thinking is that, on some situations for the rest of the function, 
there may be one too many arguments. To give a concrete example, I 
thought that if you had foo.bar(), where bar is a regular method, this 
could be adding the "this" argument one too many times. But that would 
mean "callee" is 'foo', and re-reading with fresh eyes, callee is 
supposed to be "bar", right?

>
>
>>> +    }
>>> +
>>>      if (noside == EVAL_AVOID_SIDE_EFFECTS)
>>>        {
>>>          /* If the return type doesn't look like a function type,
>>>        call an error.  This can happen if somebody tries to turn
>>>        a variable into a function call.  */
>>>
>>> -      type *ftype = callee->type ();
>>> -
>>>          if (ftype->code () == TYPE_CODE_INTERNAL_FUNCTION)
>>>        {
>>>          /* We don't know anything about what the internal
>>> @@ -666,9 +687,11 @@ operation::evaluate_funcall (struct type *expect_type,
>>>      struct type *type = callee->type ();
>>>      if (type->code () == TYPE_CODE_PTR)
>>>        type = type->target_type ();
>>> +  bool type_has_arguments
>>> +    = type->code () == TYPE_CODE_FUNC || type->code () == TYPE_CODE_METHOD;
>>>      for (int i = 0; i < args.size (); ++i)
>>>        {
>>> -      if (i < type->num_fields ())
>>> +      if (type_has_arguments && i < type->num_fields ())
>> This change also looks to be unrelated to this patch?
> It's what I described here:
>
>> The change in operation::evaluate_funcall is to make sure the type
>> fields are only used for function types, only they use them as the
>> argument types.
>   
> Before this patch it didn't matter if it used the field types in the
> evaluate calls, but since the caller type could now also be a struct,
> these would be the struct fields types, not function arguments.

Ooohh, I see.

Again, I think a code comment would help a lot. Something above the if, 
saying for example:

     "If type is a struct, num_fields would refer to the number of 
members in the type, not the number of arguments"

or similar.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
>
> Hannes
>