Re: [RFA 1/8] Add gdb_ref_ptr.h

[Pedro Alves <palves@redhat.com>]

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