[v2,1/4] Add gdbpy_borrowed_ref
Checks
Commit Message
This adds new gdbpy_opt_borrowed_ref and gdbpy_borrowed_ref classes.
These class is primarily for code "documentation" purposes -- it makes
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(+)
Comments
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
Andrew> OK, please excuse my ignorance here, I might be completely wrong, but I
Andrew> believe the line 'typename = std::is_convertible<T *, PyObject *>' is
Andrew> here for the purpose of SFINAE. I believe this was copied from
Andrew> gdb_ref_ptr.h, but I think this line might be wrong, both there and
Andrew> here.
I sent a separate patch to fix up gdb_ref_ptr.h.
Tom
>> This adds new gdbpy_opt_borrowed_ref and gdbpy_borrowed_ref classes.
>> These class is primarily for code "documentation" purposes -- it makes
Andrew> type: These CLASSES ARE primarily ....
Fixed.
Andrew> Could/should this be marked as 'const'?
Yeah, plus the other ones you pointed out.
>> + /* 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);
>> + }
Andrew> OK, please excuse my ignorance here, I might be completely wrong, but I
Andrew> believe the line 'typename = std::is_convertible<T *, PyObject *>' is
Andrew> here for the purpose of SFINAE. I believe this was copied from
Andrew> gdb_ref_ptr.h, but I think this line might be wrong, both there and
Andrew> here.
Yep, oops.
I fixed this here & sent a separate patch for gdb_ref_ptr.h.
Tom
@@ -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;
+ }
+
+ 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);
+ }
+
+ /* When converting a borrowed reference to a gdbpy_ref<>, a new
+ reference is acquired. */
+ operator gdbpy_ref<> ()
+ {
+ return gdbpy_ref<>::new_reference (m_obj);
+ }
+};
+
/* A wrapper class for Python extension objects that have a __dict__ attribute.
Any Python C object extension needing __dict__ should inherit from this