Re: [PATCH v2 1/4] Add gdbpy_borrowed_ref

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

> This adds new gdbpy_opt_borrowed_ref and gdbpy_borrowed_ref classes.
> These class is primarily for code "documentation" purposes -- it makes

type: These CLASSES ARE primarily ....

> it clear to the reader that a given reference is borrowed.  However,
> they also add a tiny bit of safety, in that conversion to gdbpy_ref<>
> will either be rejected (by the "opt" class) or acquire a new
> reference.
> ---
>  gdb/python/py-ref.h | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/gdb/python/py-ref.h b/gdb/python/py-ref.h
> index dc0b14814af..ef6b9fb427c 100644
> --- a/gdb/python/py-ref.h
> +++ b/gdb/python/py-ref.h
> @@ -41,6 +41,86 @@ struct gdbpy_ref_policy
>  template<typename T = PyObject> using gdbpy_ref
>    = gdb::ref_ptr<T, gdbpy_ref_policy>;
>  
> +/* A class representing an optional borrowed reference.  It is
> +   "optional" because NULL is a valid value.
> +
> +   This is a simple wrapper for a PyObject*.  Aside from documenting
> +   what the code does, the main advantage of using this is that
> +   conversion to a gdbpy_ref<> is prevented.
> +
> +   An optional borrowed reference is only used in situations where
> +   Python says NULL is valid.  For example, it is used as the type of
> +   the "keywords" argument to a varargs method.  Most code should
> +   prefer an ordinary gdbpy_borrowed_ref, see below.  */
> +class gdbpy_opt_borrowed_ref
> +{
> +public:
> +
> +  gdbpy_opt_borrowed_ref (PyObject *obj)
> +    : m_obj (obj)
> +  {
> +  }
> +
> +  template<typename T>
> +  gdbpy_opt_borrowed_ref (const gdbpy_ref<T> &ref)
> +    : m_obj (ref.get ())
> +  {
> +  }
> +
> +  operator PyObject * ()
> +  {
> +    return m_obj;
> +  }

Could/should this be marked as 'const'?

> +
> +  operator gdbpy_ref<> () = delete;
> +
> +protected:
> +  PyObject *m_obj;
> +};
> +
> +/* A borrowed reference that is guaranteed not to be NULL.
> +
> +   Like gdbpy_opt_borrowed_ref, this mostly serves a documentary
> +   purpose.  However, it also allows a checked cast to any subclass of
> +   PyObject, and conversion to a gdbpy_ref<> will automatically
> +   acquire a new reference -- a safety improvement over plain
> +   PyObject*.  */
> +class gdbpy_borrowed_ref : public gdbpy_opt_borrowed_ref
> +{
> +public:
> +
> +  gdbpy_borrowed_ref (PyObject *obj)
> +    : gdbpy_opt_borrowed_ref (obj)
> +  {
> +    gdb_assert (m_obj != nullptr);
> +  }
> +
> +  template<typename T>
> +  gdbpy_borrowed_ref (const gdbpy_ref<T> &ref)
> +    : gdbpy_opt_borrowed_ref (ref)
> +  {
> +    gdb_assert (m_obj != nullptr);
> +  }
> +
> +  gdbpy_borrowed_ref (std::nullptr_t) = delete;
> +
> +  /* Allow a (checked) conversion to any subclass of PyObject.  */
> +  template<typename T,
> +	   typename = std::is_convertible<T *, PyObject *>>
> +  operator T * ()
> +  {
> +    gdb_assert (PyObject_TypeCheck (m_obj, T::corresponding_object_type));
> +    return static_cast<T *> (m_obj);
> +  }

OK, please excuse my ignorance here, I might be completely wrong, but I
believe the line 'typename = std::is_convertible<T *, PyObject *>' is
here for the purpose of SFINAE.  I believe this was copied from
gdb_ref_ptr.h, but I think this line might be wrong, both there and
here.

I think std::is_convertible is either true or false, or the type will be
true_type or false_type, but in all cases, I think the type is well
defined, so there will never be substitution failure.

I think what you need here is:

  typename = gdb::Requires<std::is_convertible<T *, PyObject *>>

But it might be that I'm just not understanding what's going on here, in
which case, feel free to correct me.

> +
> +  /* When converting a borrowed reference to a gdbpy_ref<>, a new
> +     reference is acquired.  */
> +  operator gdbpy_ref<> ()
> +  {
> +    return gdbpy_ref<>::new_reference (m_obj);
> +  }

Again with the 'const' maybe?

Thanks,
Andrew

> +};
> +
>  /* A wrapper class for Python extension objects that have a __dict__ attribute.
>  
>     Any Python C object extension needing __dict__ should inherit from this
>
> -- 
> 2.49.0