Re: [PATCH v2 3/4] Add wrappers for Python implementation functions and methods

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>> Note this patch is not 100% complete.  There should be one more
>>> wrapper for case where a method takes a single argument (though we
>>> probably cannot use METH_O unfortunately).  There may be some other
>>> holes as well.
>
> Andrew> Maybe reword this so it doesn't imply that THIS patch is not complete,
> Andrew> but rather the implementation as a whole is not complete and will
> Andrew> require future follow on patches.  What you've got is good enough to
> Andrew> start using it.
>
> I updated the text a bit.
>
>>> +template<typename T, typename = std::is_integral<T>>
>
> Andrew> I think the SFINAE part here is wrong, like in the previous commit.  I
> Andrew> think gdb::Requires<std::is_integral<T>> might be what you mean.
>
> Fixed.
>
> Andrew> None of the to_python functions check their return values for error, for
> Andrew> example PyUnicode_Decode can fail and return NULL, but you don't check
> Andrew> for this.
>
> Andrew> But this is OK.  to_python is only used at the point where we transition
> Andrew> back from GDB's C++ code to the Python internals, so if PyUnicode_Decode
> Andrew> (for example) returns NULL and sets an exception, this will be caught by
> Andrew> Python.
>
> Andrew> I have two pieces of feedback on this:
>
> Andrew>  1. I think this should be explicitly called out in the comment above
> Andrew>     the to_python functions, rather than making everyone figure out that
> Andrew>     this is not a mistake.
>
> I updated the comment that precedes the to_python functions as a whole.
>
>>> +  /* Note that this cannot fail.  */
>
> Andrew>     Is (IMHO) confusing.  It implies the lack of error checking here is
> Andrew>     because PyBool_FromLong cannot fail, which is why, when I look at
> Andrew>     later to_python functions which include calls that *can* fail, I
> Andrew>     asked myself, where's the error checking.
>
> Andrew>     I think this comment should just go.
>
> Deleted.
>
>>> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
>
> Andrew> typo: ".... then THEN calls ..."
>
> Fixed.
>
>>> +/* An instantiation of this function is used when calling a gdb method
>>> +   from Python.  It accepts some number of arguments (normally
>>> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
>>> +   the underlying function F.  Any exceptions are caught and
>>> +   converted, and the return value of F is converted to a Python
>>> +   object as appropriate.  */
>
> Andrew> This comment needs updating.  It also include the 'then then' typo from
> Andrew> the wrapped_function comment, but also references function F, when it
> Andrew> should be taking about method CLASS::METH or maybe just METH?  Anyway,
> Andrew> certainly not F.
>
> Fixed.
>
>>> +/* A function that wraps a "repr" or "str" method.  */
>>> +template<typename C, auto M>
>
> Andrew> In wrap_setter below you are explicit about the signature of M.  I much
> Andrew> prefer the explicit form, but here in wrap_repr and in wrap_getter you
> Andrew> use 'auto'.  Could we switch to the explicit form in these two too?
>
> There are two issues with changing.
>
> One is that while a setter should probably just return void, a getter
> could return anything.  And, requiring a specific return type for tp_str
> or tp_repr seems a bit heavy, like maybe it would be convenient to
> return std::string in some spots or gdbpy_ref<> in others.
>
> The other issue is that 'auto' means it automatically accepts const- or
> non-const-methods.  This can be handled by overloads of course.
>
> Anyway I left this as is but we can discuss further if you want.

No, that's fine, thanks for explaining the benefit here.  I'm perfectly
happy with 'auto' when it's serving a good purpose like this.

Thanks,
Andrew