Re: [PATCH] Replace some xmalloc-family functions with XNEW-family ones

[Doug Evans <dje at google dot com>]

Simon Marchi writes:
 > On 15-08-26 03:44 PM, Pedro Alves wrote:
 > > On 08/26/2015 08:24 PM, Simon Marchi wrote:
 > >
 > >> I was able to test with just a few targets, those for which I was  
able to
 > >> build a toolchain with crosstool-ng (arm and mips).  I did mingw32 as  
well.
 > >
 > > Thanks.
 > >
 > >> I applied for an access to the gcc compile farm, but I'm still  
waiting.
 > >>
 > >> I don't think I can do better at the moment.  Is there still something
 > >> holding back the patch?
 > >
 > > I think you should go ahead.  If anything breaks, it should be a  
trivial fix.
 > >
 > > Thanks,
 > > Pedro Alves
 >
 > Alright, pushed a slightly modified (rebased) version:
 >
 >
 > >From 8d7493201cf01c9836403695f67f7e157341bfd5 Mon Sep 17 00:00:00 2001
 > From: Simon Marchi <simon.marchi@ericsson.com>
 > Date: Wed, 26 Aug 2015 17:16:07 -0400
 > Subject: [PATCH] Replace some xmalloc-family functions with XNEW-family  
ones
 >
 > This patch is part of the make-gdb-buildable-in-C++ effort.  The idea is
 > to change some calls to the xmalloc family of functions to calls to the
 > equivalents in the XNEW family.  This avoids adding an explicit cast, so
 > it keeps the code a bit more readable.  Some of them also map relatively
 > well to a C++ equivalent (XNEW (struct foo) -> new foo), so it will be
 > possible to do scripted replacements if needed.
 >
 > I only changed calls that were obviously allocating memory for one or
 > multiple "objects".  Allocation of variable sizes (such as strings or
 > buffer handling) will be for later (and won't use XNEW).
 >
 >   - xmalloc (sizeof (struct foo)) -> XNEW (struct foo)
 >   - xmalloc (num * sizeof (struct foo)) -> XNEWVEC (struct foo, num)
 >   - xcalloc (1, sizeof (struct foo)) -> XCNEW (struct foo)
 >   - xcalloc (num, sizeof (struct foo)) -> XCNEWVEC (struct foo, num)
 >   - xrealloc (p, num * sizeof (struct foo) -> XRESIZEVEC (struct foo, p,  
num)
 >   - obstack_alloc (ob, sizeof (struct foo)) -> XOBNEW (ob, struct foo)
 >   - obstack_alloc (ob, num * sizeof (struct foo)) -> XOBNEWVEC (ob,  
struct foo, num)
 >   - alloca (sizeof (struct foo)) -> XALLOCA (struct foo)
 >   - alloca (num * sizeof (struct foo)) -> XALLOCAVEC (struct foo, num)
 >
 > Some instances of xmalloc followed by memset to zero the buffer were
 > replaced by XCNEW or XCNEWVEC.
 >
 > I regtested on x86-64, Ubuntu 14.04, but the patch touches many
 > architecture-specific files.  For those I'll have to rely on the
 > buildbot or people complaining that I broke their gdb.
 >
 > gdb/ChangeLog:
 >
 > 	* aarch64-linux-nat.c (aarch64_add_process): Likewise.
 > 	* aarch64-tdep.c (aarch64_gdbarch_init): Likewise.
 > 	* ada-exp.y (write_ambiguous_var): Likewise.
 > 	* ada-lang.c (resolve_subexp): Likewise.
 > 	(user_select_syms): Likewise.
 > 	(assign_aggregate): Likewise.
 > 	(ada_evaluate_subexp): Likewise.
 > 	(cache_symbol): Likewise.

Hi.
First off, thanks for doing this!
I don't want to discourage such cleanups by nitpicking.

A couple of comments.

1) Apologies you felt you needed to write such a long changelog
entry. IMO it's not necessary. There's no doubt a rule that
says such verbosity is indeed necessary, but I'd like to think we
can bend the rules now and then for cases such as this.
I'm not sure what I'd have done differently, I'd have to
go through the patch. If all functions in a file were updated
I'd be ok with:

	* ada-lang.c (*): Likewise.

There is precedent for doing that.
And even if not all functions were updated
(and therefore "(*)" is inaccurate),
I think a properly worded introductory sentence
would make it ok.

2) The changelog entry needs to be made more readable.
I'd be ok with just adding a line at the top.
E.g.,

	Change some calls to the xmalloc family of functions
	to calls to the equivalents in the XNEW family.
	* aarch64-linux-nat.c (aarch64_add_process): Likewise.
	...

As it currently is all I see is "Likewise." throughout
and the reader has no idea what it refers to.
[Yeah, the reader can go into the git logs,
but they shouldn't have to.]