Re: RFC: prototype of C extensions using the Python limited API

[Tom Tromey <tom@tromey.com>]

>>>>> Matthieu Longo via Gdb <gdb@sourceware.org> writes:

>> Could you please review it so that we validate this new approach
>> before I migrate all my previous patches to it ?

Sorry about the delay on this.

I finally found time today to read through this.

I'm not sure I really understood it.  There is a lot of stuff here, and
I ended up trying to pick through the patch and figure out which parts
are directly from gdb and which are modified.  However this isn't really
easy to do.

It would probably help if you gave some kind of high-level overview of
what your goal is and what the main classes are that implement the goal.

Alternatively, a gdb patch series would probably be a lot simpler on the
review end.

I guess the goal is heap allocation of types and implementation of the
'traverse' stuff?  To achieve this I think the main things I would look
for in review are:

* Any new APIs not be any worse for safety than what we currently are
  working toward.  That is, automate to the extent possible, avoid error
  checks, don't pass raw PyObject* around.

* Don't have excessive classes, templates, etc.

* It's fine to put some new constraints on gdb-supplied code if it makes
  the wrappers simpler.  Like, gdb probably won't interact with Python
  type objects in the most fully general way -- it's fine to say that
  gdb will create types and not ever destroy them, if that helps at all.

> +### Issues
> +
> +1. Not sure why A_clear and B_clear are called twice. Is it due to the cycle ?
> +2. The type name is `(null)` because `PyType_GetFullyQualifiedName()` returns `NULL` for an unknown reason. Please could you explain me why ?
> +3. There is an unexpected error `AttributeError: __module__`. I don't understand where it comes from.

I don't know the answer to any of these.

> +  template<typename P = Policy, typename = std::enable_if_t<
> +	   std::is_same<P, gdbpy_ref_policy>::value>>
> +  int visit (typename P::visitproc visit, void *arg) noexcept
> +  {
> +    return P::visit (m_obj, visit, arg);
> +  }

I didn't understand why this is here.

> +
> +  template<typename P = Policy, typename = std::enable_if_t<
> +	   std::is_same<P, gdbpy_ref_policy>::value>>
> +  void clear (P * = nullptr) noexcept
> +  {
> +    P::clear (&m_obj);
> +  }

Earlier you said this was for storing a 'gdbpy_ref<> *' in a Python
object.  I somewhat feel that's to be avoided and we should just use
PyObject*.

Though thinking about it, I guess it's hard to articulate why.  I
suppose a pointer to a smart pointer feels strange.  It's unfortunate we
can't use the reference wrappers in the gdb classes for the time being;
but since we can't I suppose it's perhaps best to follow Python
conventions and just document that the fields are owned references.

> +
> +template <class TypeWrapper>
> +struct gdbpy_object_type
> +{
> +private:
> +  PyTypeObject *pytype_ () const noexcept

If this is a wrapper I think "gdbpy_type_object" might be a nicer name.

> +struct gdbpy_object_type_ref:
> +  public gdbpy_object_type <gdbpy_object_type_ref>,
> +  public gdbpy_ref <>

> +struct gdbpy_object_type_bref:
> +  public gdbpy_object_type <gdbpy_object_type_bref>,
> +  public gdbpy_borrowed_ref <>

> +struct gdbpy_py_obj_type_bref:
> +  public gdbpy_object_type <gdbpy_py_obj_type_bref>,
> +  public gdbpy_borrowed_ref <PyTypeObject>

I didn't understand why these are all needed, or what their names mean.

Tom