Using gnu::unique_ptr to avoid manual cleanups (was Re: [PATCH 2/2] use unique_ptr some)

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2017-07-31 at 19:46 -0400, tbsaunde+gcc@tbsaunde.org wrote:
> From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>
> 
> gcc/ChangeLog:
> 
> 2017-07-31  Trevor Saunders  <tbsaunde+gcc@tbsaunde.org>
> 
> 	* cse.c (find_comparison_args): Make visited a unique_ptr.
> 	* lto-streamer-out.c (write_global_references): Make data a
> 	unique_ptr.
> 	* tree-cfg.c (move_sese_region_to_fn): Make several variables
> 	unique_ptrs.

[...snip....]

Trevor posted this patch back in July:

  https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02085.html

to use a unique_ptr (compatible with pre-C++-11) to remove explicit
"free"s, potentially simplifying cleanup logic and reducing the risk of
memory leaks.

I have a version of the patch, updated to use the "gnu::unique_ptr" now
in trunk (rather than his "gtl::")...

> diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
> index 41fba318cb5..61576c30266 100644
> --- a/gcc/lto-streamer-out.c
> +++ b/gcc/lto-streamer-out.c
> @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "config.h"
>  #include "system.h"
>  #include "coretypes.h"
> +#include "unique-ptr.h"
>  #include "backend.h"
>  #include "target.h"
>  #include "rtl.h"
> @@ -2434,7 +2435,7 @@ write_global_references (struct output_block
> *ob,
>    const uint32_t size = lto_tree_ref_encoder_size (encoder);
>  
>    /* Write size and slot indexes as 32-bit unsigned numbers. */
> -  uint32_t *data = XNEWVEC (uint32_t, size + 1);
> +  gtl::unique_ptr<uint32_t[]> data (new uint32_t[size + 1]);

...but I was wondering if this kind of thing should continue to use
XNEWVEC rather than "new"?

Given that we build with -fno-exceptions, what are we guaranteed about
what happens when "new" fails?  (am I right in thinking that a failed
allocation returns NULL in this case?).  Is XNEWVEC preferable here?

i.e. do we prefer:

(A)

  gnu::unique_ptr<uint32_t[]> data (new uint32_t[size + 1]);

(implicitly delete-ing the data, with an access-through-NULL if the
allocation fails)

or

(B):
  gnu::unique_xmalloc_ptr<uint32_t[]> data (XNEWVEC (uint32_t, size + 1));

(implicitly free-ing the data, with an xmalloc_failed and "exit" if the
allocation fails)


or the status quo:

(C):

  uint32_t *data = XNEWVEC (uint32_t, size + 1);
  [...]
  free (data); // by hand on every path in which "data" goes out of scope

?

Personally I prefer the syntax of (A).

[...snip...]


Thanks
Dave