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

[Pedro Alves <palves at redhat dot com>]

On 08/18/2015 09:49 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Hi Simon,
> Thanks for doing this.
> 
>> 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 am not against this patch, and I think this patch is useful to shorten
> the code in some cases.  However, I don't think we really need it to
> make GDB buildable in C++, right?  We can still use xmalloc in a C++
> program (xmalloc is still in use in GCC).

Right.  The issue is that in C++, we need to cast the result of xmalloc.
E.g.:

 -  struct foo *f = xmalloc (sizeof (struct foo));
 +  struct foo *f = (struct foo *) xmalloc (sizeof (struct foo));

We get away without the casts today because building in C++ mode
currently forces -fpermissive (which is a temporary hack), which makes
the missing cast a warning instead of an error.

The XNEW-family of macros hide the cast away, and adds type-safety.
E.g., this compiles and does the wrong thing:

  struct foo *f = (struct foo *) xmalloc (sizeof (struct bar));

This too, compiles and does the wrong thing:

  struct foo *f = (struct foo *) xmalloc (sizeof (struct foo *));

these don't compile:

  struct foo *f = XNEW (struct bar);
  struct foo *f = XNEW (struct foo *);

So even without C++ in the picture, they could be considered an
improvement, because they're typo-safer.  The downside is the
potential confusion to newcomers due to more obfuscation.  We already
use XNEW&co in the tree today though, so it's not really a new thing.

> 
> XNEW/xmalloc -> new or struct -> class conversion should be done per
> data structure rather than globally like this patch does.  We only
> convert C-inheritance data structures like breakpoint_ops, varobj, etc,
> to C++ class, and leave the rest there unless it is necessary to change
> them to C++ class.  This is my personal understanding, and I'd like to
> hear others thoughts.

Agreed.  We shouldn't go and do XNEW/xmalloc -> new/new[] all over the
tree just for the sake of it.  It will naturally happen sometimes that
we'll refactor code and in the process xmalloc -> new happens, but
I don't think that that should be a goal in itself.

> 
>>
>> 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 am not against this conversion.  If we accept this change, are
> xmalloc/xcalloc/... not allowed to use, and XNEW/XNEWVEC/... should be
> used instead.  The restriction is not necessary to me.

I'm super fine with making XNEW not be a requirement, if people are
OK with the extra casts.  I was myself thinking of leaving these casts
issues for one of the last steps of the C++ conversion in master (in the
branch, it actually helps that it's one of the first in the series, as it
gets the warnings out of the way), and just rely on the auto-insert-casts
script.  I actually had a couple (much smaller) patches that did
xmalloc -> XNEW in my branch early on, but got rid of them in one of the
latest rebases, as it seemed pointless when I was meaning to propose
casts in many other places.  :-P

So IMO, kudos to Simon for being brave and doing all this.  Seems like a
good amount of work, and the result stands on its own, even without
considering C++.  The patch looks good to me, FWIW.

Thanks,
Pedro Alves