[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH 31/88] QMP: use g_new() family of
From: |
Eric Blake |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH 31/88] QMP: use g_new() family of functions |
Date: |
Mon, 9 Oct 2017 13:04:35 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 10/09/2017 03:11 AM, Dr. David Alan Gilbert wrote:
>>>
>>> - info = g_malloc0(sizeof(*info));
>>> + info = g_new0(CommandInfoList, 1);
>>> info->value = g_malloc0(sizeof(*info->value));
>>> info->value->name = g_strdup(cmd->name);
>>> info->next = *list;
>>
>> I'm not convinced rewriting
>>
>> LHS = g_malloc(sizeof(*LHS));
>>
>> to
>>
>> LHS = g_new(T, 1);
>>
>> where T is the type of LHS is worth the trouble. The code before the
>> rewrite is pretty idiomatic. There's no possibility of integer overflow
>> g_new() could avoid. The types are obviously correct, so the additional
>> type checking is quite unlikely to catch anything. That leaves the
>> consistency argument. I'm willing to hear it, but I feel it should be
>> heard in a patch series that does nothing else.
>
> The 'obviously correct' is the dodgy part of the argument here.
> How many bugs do we right that are obviously wrong?
>
> t.c:13:20: warning: initialization from incompatible pointer type
> [-Wincompatible-pointer-types]
> struct c *pc = g_new(struct b, 1);
>
> seems good to me.
Yes, that's a GOOD warning, and it proves that we DO get type safety
when we convert:
LHS = g_malloc(sizeof(type))
into
LHS = g_new(type, 1)
but that's not what Markus was pointing out. When we already have the
correctly-typed object on the right hand side, as in:
LHS = g_malloc(sizeof(*LHS))
then the compiler will always give us the correct type of LHS, whereas with:
LHS = g_new(type, 1)
we have to manually update the line if the type of LHS changes.
Thus, converting malloc(sizeof(type)) into new(type, 1) is a no-brainer,
but converting malloc(sizeof(*expr)) needs justification. I'm not
necessarily opposed to the conversion (if our justification is
consistency, and where HACKING documents our style and where we have
scripts that let us easily preserve our style), but I'm not sure I see
the requisite justification in the current iteration of this series.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-trivial] [PATCH 26/88] S390: use g_new() family of functions, (continued)
- [Qemu-trivial] [PATCH 28/88] disas: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 27/88] SH4: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 29/88] SPARC: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 30/88] QEMU Guest Agent: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 32/88] QObject: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 31/88] QMP: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 33/88] qom: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 34/88] qapi: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 35/88] Record/replay: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 36/88] SLIRP: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 37/88] TCG: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 38/88] VFIO: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 39/88] hw/i386: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-trivial] [PATCH 40/88] hw/xen: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06