[RFA,1/8] Add gdb_ref_ptr.h
Commit Message
This adds a new gdb_ref_ptr.h, that implements a reference-counting
smart pointer class, where the user of the class supplies a
reference-counting policy object.
This class will be used in the next patch, which changes most explicit
BFD reference counts to use this new type. Meanwhile, this patch
changes gdbpy_ref to be a specialization of this new class.
This change required adding new nullptr_t overloads some operators in
gdb_ref_ptr.h. I suspect this was needed because some Python header
redefines NULL, but I'm not certain.
2016-11-28 Tom Tromey <tom@tromey.com>
* common/gdb_ref_ptr.h: New file.
* python/py-ref.h (struct gdbpy_ref_policy): New.
(gdbpy_ref): Now a typedef.
---
gdb/ChangeLog | 6 ++
gdb/common/gdb_ref_ptr.h | 209 +++++++++++++++++++++++++++++++++++++++++++++++
gdb/python/py-ref.h | 135 ++----------------------------
3 files changed, 224 insertions(+), 126 deletions(-)
create mode 100644 gdb/common/gdb_ref_ptr.h
Comments
On 11/29/2016 05:05 AM, Tom Tromey wrote:
> This adds a new gdb_ref_ptr.h, that implements a reference-counting
> smart pointer class, where the user of the class supplies a
> reference-counting policy object.
>
> This class will be used in the next patch, which changes most explicit
> BFD reference counts to use this new type. Meanwhile, this patch
> changes gdbpy_ref to be a specialization of this new class.
>
> This change required adding new nullptr_t overloads some operators in
> gdb_ref_ptr.h. I suspect this was needed because some Python header
> redefines NULL, but I'm not certain.
Can you please remove them and see what breaks? Odd that this would be
something about Python, given the class is being used in Python code
today?
BTW, it's worth noting that there's a proposal to add (an oddly named)
policy-based intrusive ref counting smart pointer to the standard:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0468r0.html
Thanks,
Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> Can you please remove them and see what breaks? Odd that this would be
Pedro> something about Python, given the class is being used in Python code
Pedro> today?
Yes, it's odd. If I comment out the nullptr_t overloads, I get build
failures like:
../../binutils-gdb/gdb/python/py-exitedevent.c:31:20: error: no match for ‘operator==’ (operand types are ‘gdbpy_ref {aka gdb::ref_ptr<_object, gdbpy_ref_policy>}’ and ‘long int’)
if (exited_event == NULL)
^
../../binutils-gdb/gdb/common/gdb_ref_ptr.h:183:13: note: template argument deduction/substitution failed:
In file included from ../../binutils-gdb/gdb/python/py-ref.h:23:0,
from ../../binutils-gdb/gdb/python/py-event.h:27,
from ../../binutils-gdb/gdb/python/py-event.c:21:
../../binutils-gdb/gdb/common/gdb_ref_ptr.h:171:13: note: candidate: template<class T, class POLICY> bool gdb::operator==(const T*, const gdb::ref_ptr<T, POLICY>&)
inline bool operator== (const T *self, const ref_ptr<T, POLICY> &other)
^~~~~~~~
I don't understand why it picks this particular candidate, but
re-reading it now I think my earlier theory is wrong.
Tom
On 12/02/2016 05:46 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> Can you please remove them and see what breaks? Odd that this would be
> Pedro> something about Python, given the class is being used in Python code
> Pedro> today?
>
> Yes, it's odd. If I comment out the nullptr_t overloads, I get build
> failures like:
>
> ../../binutils-gdb/gdb/python/py-exitedevent.c:31:20: error: no match for ‘operator==’ (operand types are ‘gdbpy_ref {aka gdb::ref_ptr<_object, gdbpy_ref_policy>}’ and ‘long int’)
> if (exited_event == NULL)
> ^
> ../../binutils-gdb/gdb/common/gdb_ref_ptr.h:183:13: note: template argument deduction/substitution failed:
> In file included from ../../binutils-gdb/gdb/python/py-ref.h:23:0,
> from ../../binutils-gdb/gdb/python/py-event.h:27,
> from ../../binutils-gdb/gdb/python/py-event.c:21:
> ../../binutils-gdb/gdb/common/gdb_ref_ptr.h:171:13: note: candidate: template<class T, class POLICY> bool gdb::operator==(const T*, const gdb::ref_ptr<T, POLICY>&)
> inline bool operator== (const T *self, const ref_ptr<T, POLICY> &other)
> ^~~~~~~~
>
>
> I don't understand why it picks this particular candidate, but
I think gcc will list you all candidates, mentioning why each one
can't work. I.e., it likely tells you more further below?
> re-reading it now I think my earlier theory is wrong.
Does it still happen if you remove the cstddef include?
Do you have your code in some branch? It seems none of the
gdbpy_ref stuff is in master yet.
Thanks,
Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> I think gcc will list you all candidates, mentioning why each one
Pedro> can't work. I.e., it likely tells you more further below?
Yeah, it does, I missed that.
Pedro> Does it still happen if you remove the cstddef include?
Yes.
Pedro> Do you have your code in some branch? It seems none of the
Pedro> gdbpy_ref stuff is in master yet.
I pushed it to py-cxx-changes on my github account.
I haven't pushed anything in since I have one giant branch and there
were some issues with a couple individual patches. The final two
patches on that branch aren't submitted yet, they are the fixes I need
to fold in and resubmit.
Tom
On 12/02/2016 07:52 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> I think gcc will list you all candidates, mentioning why each one
> Pedro> can't work. I.e., it likely tells you more further below?
>
> Yeah, it does, I missed that.
Ah, trying locally and getting at the compile log helped. The overload
we'd expect to work, doesn't:
..../src/gdb/solib-darwin.c: In function ‘void darwin_solib_get_all_image_info_addr_at_init(darwin_info*)’:
..../src/gdb/solib-darwin.c:467:16: error: no match for ‘operator!=’ (operand types are ‘gdb_bfd_ref_ptr {aka gdb::ref_ptr<bfd, gdb_bfd_ref_policy>}’ and ‘long int’)
if (dyld_bfd != NULL)
^
[...]
..../src/gdb/common/gdb_ref_ptr.h:212:13: note: candidate: template<class T, class POLICY> bool gdb::operator!=(const gdb::ref_ptr<T, POLICY>&, const T*)
inline bool operator!= (const ref_ptr<T, POLICY> &self, const T *other)
^
..../src/gdb/common/gdb_ref_ptr.h:212:13: note: template argument deduction/substitution failed:
..../src/gdb/solib-darwin.c:467:19: note: mismatched types ‘const T*’ and ‘long int’
if (dyld_bfd != NULL)
^
[...]
That's simply because template type deduction, which happens
before overload resolution, does not consider implicit conversions.
And then there's no overload in the overload set that satisfied
the operation.
Before the gdb_ref_ptr.h patch (this email thread), gdbpy_ref
is not a template, so:
inline bool operator!= (const gdbpy_ref &self, const PyObject *other);
is left in the overload set, at which point implicit conversions can
apply.
I was a bit mystified about why my gdb_unique_ptr shim had == NULL
working without nullptr_t overloads, but I remember now. It was
because it was using the safe bool idiom to make it work. I.e.,
adding this to ref_ptr would make operator==/operator!= work without
the nullptr_t overloads too:
/* "explicit operator bool ()" emulation using the safe bool
idiom. */
private:
typedef void (ref_ptr::*explicit_operator_bool) () const;
void this_type_does_not_support_comparisons () const {}
public:
operator explicit_operator_bool () const
{
return (m_obj != NULL
? &ref_ptr::this_type_does_not_support_comparisons
: 0);
}
With this, despite the fact that no operator== candidate template
matches, the compile still manages to call the built-in, non-template:
bool operator==(member function ptr, long);
> Pedro> Do you have your code in some branch? It seems none of the
> Pedro> gdbpy_ref stuff is in master yet.
>
> I pushed it to py-cxx-changes on my github account.
Thanks, that helped.
So the conclusion is that the nullptr_t overloads are necessary because
ref_ptr is now a template.
BTW, notice that both retain_ptr (that proposal I linked to), and unique_ptr
have these same overloads.
Thanks,
Pedro Alves
On 11/29/2016 05:05 AM, Tom Tromey wrote:
> This adds a new gdb_ref_ptr.h, that implements a reference-counting
> smart pointer class, where the user of the class supplies a
> reference-counting policy object.
>
> This class will be used in the next patch, which changes most explicit
> BFD reference counts to use this new type. Meanwhile, this patch
> changes gdbpy_ref to be a specialization of this new class.
>
> This change required adding new nullptr_t overloads some operators in
> gdb_ref_ptr.h. I suspect this was needed because some Python header
> redefines NULL, but I'm not certain.
>
> 2016-11-28 Tom Tromey <tom@tromey.com>
>
> * common/gdb_ref_ptr.h: New file.
> * python/py-ref.h (struct gdbpy_ref_policy): New.
> (gdbpy_ref): Now a typedef.
> ---
> gdb/ChangeLog | 6 ++
> gdb/common/gdb_ref_ptr.h | 209 +++++++++++++++++++++++++++++++++++++++++++++++
> gdb/python/py-ref.h | 135 ++----------------------------
> 3 files changed, 224 insertions(+), 126 deletions(-)
> create mode 100644 gdb/common/gdb_ref_ptr.h
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 8cbe8ad..f455f7f 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,11 @@
> 2016-11-28 Tom Tromey <tom@tromey.com>
>
> + * common/gdb_ref_ptr.h: New file.
> + * python/py-ref.h (struct gdbpy_ref_policy): New.
> + (gdbpy_ref): Now a typedef.
> +
> +2016-11-28 Tom Tromey <tom@tromey.com>
> +
> * utils.h (make_cleanup_htab_delete): Don't declare.
> * utils.c (do_htab_delete_cleanup, make_cleanup_htab_delete):
> Remove.
> diff --git a/gdb/common/gdb_ref_ptr.h b/gdb/common/gdb_ref_ptr.h
> new file mode 100644
> index 0000000..cc8ba94
> --- /dev/null
> +++ b/gdb/common/gdb_ref_ptr.h
> @@ -0,0 +1,209 @@
> +/* Reference-counted smart pointer class
> +
> + Copyright (C) 2016 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#ifndef GDB_REF_PTR_H
> +#define GDB_REF_PTR_H
> +
> +#include <cstddef>
> +
> +namespace gdb
> +{
> +
> +/* An instance of this class either holds a reference to a
> + reference-counted object or is "NULL". Reference counting is
> + handled externally by a policy class. If the object holds a
> + reference, then when the object is destroyed, the reference is
> + decref'd.
> +
> + Normally an instance is constructed using a pointer. This sort of
> + initialization lets this class manage the lifetime of that
> + reference.
> +
> + Assignment and copy construction will make a new reference as
> + appropriate. Assignment from a plain pointer is disallowed to
> + avoid confusion about whether this acquires a new reference;
> + instead use the "reset" method -- which, like the pointer
> + constructor, transfers ownership.
> +
> + The policy class must provide two static methods:
> + void incref (T *);
> + void decref (T *);
> +*/
> +template<typename T, typename POLICY>
> +class ref_ptr
BTW, I noticed today that the GCC standards say:
"Template parameter names should use CamelCase, following the C++
Standard."
and they do seem to follow it. I think it's a good idea.
Would you mind adjusting accordingly?
I also noticed the "self/other" naming in the global operators:
+template<typename T, typename POLICY>
+inline bool operator== (const ref_ptr<T, POLICY> &self,
+ const ref_ptr<T, POLICY> &other)
+{
I'd find it pedantically more correct to write lhs/rhs, since
there's no actual this/self here. Could you tweak that too?
Thanks,
Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> BTW, I noticed today that the GCC standards say:
Pedro> "Template parameter names should use CamelCase, following the C++
Pedro> Standard."
Pedro> and they do seem to follow it. I think it's a good idea.
Pedro> Would you mind adjusting accordingly?
No problem, I did that.
FWIW my personal style is also to use CamelCase for macro arguments.
Pedro> I also noticed the "self/other" naming in the global operators:
Pedro> +template<typename T, typename POLICY>
Pedro> +inline bool operator== (const ref_ptr<T, POLICY> &self,
Pedro> + const ref_ptr<T, POLICY> &other)
Pedro> +{
Pedro> I'd find it pedantically more correct to write lhs/rhs, since
Pedro> there's no actual this/self here. Could you tweak that too?
I did this too.
Tom
@@ -1,5 +1,11 @@
2016-11-28 Tom Tromey <tom@tromey.com>
+ * common/gdb_ref_ptr.h: New file.
+ * python/py-ref.h (struct gdbpy_ref_policy): New.
+ (gdbpy_ref): Now a typedef.
+
+2016-11-28 Tom Tromey <tom@tromey.com>
+
* utils.h (make_cleanup_htab_delete): Don't declare.
* utils.c (do_htab_delete_cleanup, make_cleanup_htab_delete):
Remove.
new file mode 100644
@@ -0,0 +1,209 @@
+/* Reference-counted smart pointer class
+
+ Copyright (C) 2016 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef GDB_REF_PTR_H
+#define GDB_REF_PTR_H
+
+#include <cstddef>
+
+namespace gdb
+{
+
+/* An instance of this class either holds a reference to a
+ reference-counted object or is "NULL". Reference counting is
+ handled externally by a policy class. If the object holds a
+ reference, then when the object is destroyed, the reference is
+ decref'd.
+
+ Normally an instance is constructed using a pointer. This sort of
+ initialization lets this class manage the lifetime of that
+ reference.
+
+ Assignment and copy construction will make a new reference as
+ appropriate. Assignment from a plain pointer is disallowed to
+ avoid confusion about whether this acquires a new reference;
+ instead use the "reset" method -- which, like the pointer
+ constructor, transfers ownership.
+
+ The policy class must provide two static methods:
+ void incref (T *);
+ void decref (T *);
+*/
+template<typename T, typename POLICY>
+class ref_ptr
+{
+ public:
+
+ /* Create a new NULL instance. */
+ ref_ptr ()
+ : m_obj (NULL)
+ {
+ }
+
+ /* Create a new instance. OBJ is a reference, management of which
+ is now transferred to this class. */
+ explicit ref_ptr (T *obj)
+ : m_obj (obj)
+ {
+ }
+
+ /* Copy another instance. */
+ ref_ptr (const ref_ptr &other)
+ : m_obj (other.m_obj)
+ {
+ if (m_obj != NULL)
+ POLICY::incref (m_obj);
+ }
+
+ /* Transfer ownership from OTHER. */
+ ref_ptr (ref_ptr &&other)
+ : m_obj (other.m_obj)
+ {
+ other.m_obj = NULL;
+ }
+
+ /* Destroy this instance. */
+ ~ref_ptr ()
+ {
+ if (m_obj != NULL)
+ POLICY::decref (m_obj);
+ }
+
+ /* Copy another instance. */
+ ref_ptr &operator= (const ref_ptr &other)
+ {
+ /* Do nothing on self-assignment. */
+ if (this != &other)
+ {
+ reset (other.m_obj);
+ if (m_obj != NULL)
+ POLICY::incref (m_obj);
+ }
+ return *this;
+ }
+
+ /* Transfer ownership from OTHER. */
+ ref_ptr &operator= (ref_ptr &&other)
+ {
+ /* Do nothing on self-assignment. */
+ if (this != &other)
+ {
+ reset (other.m_obj);
+ other.m_obj = NULL;
+ }
+ return *this;
+ }
+
+ /* Change this instance's referent. OBJ is a reference, management
+ of which is now transferred to this class. */
+ void reset (T *obj)
+ {
+ if (m_obj != NULL)
+ POLICY::decref (m_obj);
+ m_obj = obj;
+ }
+
+ /* Return this instance's referent without changing the state of
+ this class. */
+ T *get () const
+ {
+ return m_obj;
+ }
+
+ /* Return this instance's referent, and stop managing this
+ reference. The caller is now responsible for the ownership of
+ the reference. */
+ T *release ()
+ {
+ T *result = m_obj;
+
+ m_obj = NULL;
+ return result;
+ }
+
+ private:
+
+ T *m_obj;
+};
+
+template<typename T, typename POLICY>
+inline bool operator== (const ref_ptr<T, POLICY> &self,
+ const ref_ptr<T, POLICY> &other)
+{
+ return self.get () == other.get ();
+}
+
+template<typename T, typename POLICY>
+inline bool operator== (const ref_ptr<T, POLICY> &self, const T *other)
+{
+ return self.get () == other;
+}
+
+template<typename T, typename POLICY>
+inline bool operator== (const ref_ptr<T, POLICY> &self, const std::nullptr_t)
+{
+ return self.get () == nullptr;
+}
+
+template<typename T, typename POLICY>
+inline bool operator== (const T *self, const ref_ptr<T, POLICY> &other)
+{
+ return self == other.get ();
+}
+
+template<typename T, typename POLICY>
+inline bool operator== (const std::nullptr_t, const ref_ptr<T, POLICY> &other)
+{
+ return nullptr == other.get ();
+}
+
+template<typename T, typename POLICY>
+inline bool operator!= (const ref_ptr<T, POLICY> &self,
+ const ref_ptr<T, POLICY> &other)
+{
+ return self.get () != other.get ();
+}
+
+template<typename T, typename POLICY>
+inline bool operator!= (const ref_ptr<T, POLICY> &self, const T *other)
+{
+ return self.get () != other;
+}
+
+template<typename T, typename POLICY>
+inline bool operator!= (const ref_ptr<T, POLICY> &self, const std::nullptr_t)
+{
+ return self.get () != nullptr;
+}
+
+template<typename T, typename POLICY>
+inline bool operator!= (const T *self, const ref_ptr<T, POLICY> &other)
+{
+ return self != other.get ();
+}
+
+template<typename T, typename POLICY>
+inline bool operator!= (const std::nullptr_t, const ref_ptr<T, POLICY> &other)
+{
+ return nullptr != other.get ();
+}
+
+}
+
+#endif /* GDB_REF_PTR_H */
@@ -20,140 +20,23 @@
#ifndef GDB_PYTHON_REF_H
#define GDB_PYTHON_REF_H
-/* An instance of this class either holds a reference to a PyObject,
- or is "NULL". If it holds a reference, then when the object is
- destroyed, the PyObject is decref'd.
+#include "common/gdb_ref_ptr.h"
- Normally an instance is constructed using a PyObject*. This sort
- of initialization lets this class manage the lifetime of that
- reference.
-
- Assignment and copy construction will make a new reference as
- appropriate. Assignment from a plain PyObject* is disallowed to
- avoid confusion about whether this acquires a new reference;
- instead use the "reset" method -- which, like the PyObject*
- constructor, transfers ownership.
-*/
-class gdbpy_ref
+/* A policy class for gdb::ref_ptr for Python reference counting. */
+struct gdbpy_ref_policy
{
- public:
-
- /* Create a new NULL instance. */
- gdbpy_ref ()
- : m_obj (NULL)
- {
- }
-
- /* Create a new instance. OBJ is a reference, management of which
- is now transferred to this class. */
- explicit gdbpy_ref (PyObject *obj)
- : m_obj (obj)
- {
- }
-
- /* Copy another instance. */
- gdbpy_ref (const gdbpy_ref &other)
- : m_obj (other.m_obj)
- {
- Py_XINCREF (m_obj);
- }
-
- /* Transfer ownership from OTHER. */
- gdbpy_ref (gdbpy_ref &&other)
- : m_obj (other.m_obj)
- {
- other.m_obj = NULL;
- }
-
- /* Destroy this instance. */
- ~gdbpy_ref ()
- {
- Py_XDECREF (m_obj);
- }
-
- /* Copy another instance. */
- gdbpy_ref &operator= (const gdbpy_ref &other)
- {
- /* Do nothing on self-assignment. */
- if (this != &other)
- {
- reset (other.m_obj);
- Py_XINCREF (m_obj);
- }
- return *this;
- }
-
- /* Transfer ownership from OTHER. */
- gdbpy_ref &operator= (gdbpy_ref &&other)
+ static void incref (PyObject *ptr)
{
- /* Do nothing on self-assignment. */
- if (this != &other)
- {
- reset (other.m_obj);
- other.m_obj = NULL;
- }
- return *this;
+ Py_INCREF (ptr);
}
- /* Change this instance's referent. OBJ is a reference, management
- of which is now transferred to this class. */
- void reset (PyObject *obj)
+ static void decref (PyObject *ptr)
{
- Py_XDECREF (m_obj);
- m_obj = obj;
+ Py_DECREF (ptr);
}
-
- /* Return this instance's referent. In Python terms this is a
- borrowed pointer. */
- PyObject *get () const
- {
- return m_obj;
- }
-
- /* Return this instance's referent, and stop managing this
- reference. The caller is now responsible for the ownership of
- the reference. */
- PyObject *release ()
- {
- PyObject *result = m_obj;
-
- m_obj = NULL;
- return result;
- }
-
- private:
-
- PyObject *m_obj;
};
-inline bool operator== (const gdbpy_ref &self, const gdbpy_ref &other)
-{
- return self.get () == other.get ();
-}
-
-inline bool operator== (const gdbpy_ref &self, const PyObject *other)
-{
- return self.get () == other;
-}
-
-inline bool operator== (const PyObject *self, const gdbpy_ref &other)
-{
- return self == other.get ();
-}
-
-inline bool operator!= (const gdbpy_ref &self, const gdbpy_ref &other)
-{
- return self.get () != other.get ();
-}
-
-inline bool operator!= (const gdbpy_ref &self, const PyObject *other)
-{
- return self.get () != other;
-}
-
-inline bool operator!= (const PyObject *self, const gdbpy_ref &other)
-{
- return self != other.get ();
-}
+/* A gdb::ref_ptr that has been specialized for Python objects. */
+typedef gdb::ref_ptr<PyObject, gdbpy_ref_policy> gdbpy_ref;
#endif /* GDB_PYTHON_REF_H */