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

[Tom Tromey <tom@tromey.com>]

>> 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.

Tom