Eric Blake <ebl...@redhat.com> writes:
> On 01/31/2014 08:53 AM, Markus Armbruster wrote:
>> g_new(T, n) is safer than g_malloc(sizeof(T) * n) for two reasons.
>> One, it catches multiplication overflowing size_t. Two, it returns
>> T * rather than void *, which lets the compiler catch more type
>> errors.
>>
>> Patch created with the following Coccinelle patch, with two hunks
>> dropped from the result:
>>
>
> Looks like a reasonable formula for replacement.
>
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>
>> 186 files changed, 375 insertions(+), 414 deletions(-)
>
> Is there any easy way to enforce that this style does not creep back
> into the code base? In other words, can you do a followup patch to
> checkpatch.pl that flags use of g_malloc[0]?
There are many, many uses of g_malloc(), g_malloc0() and g_realloc()
left. The ones with a size argument of the form sizeof(E) * N could be
rewritten, too. But there are plenty of others.
For the sizeof(E) * N cases, rewriting may not be an improvement. The
pattern "p = malloc(sizeof(*p))" is pretty idiomatic.
Having checkpatch.pl match the unwanted patterns sizeof(T), sizeof(T)*N,
and their variations seems infeasible to me, because deciding whether T
is a type is beyond its capabilities.
Ideas?
>> @@ -658,7 +658,7 @@ int qcow2_snapshot_list(BlockDriverState *bs,
>> QEMUSnapshotInfo **psn_tab)
>> return s->nb_snapshots;
>> }
>>
>> - sn_tab = g_malloc0(s->nb_snapshots * sizeof(QEMUSnapshotInfo));
>> + sn_tab = g_new0(QEMUSnapshotInfo, s->nb_snapshots);
>> for(i = 0; i < s->nb_snapshots; i++) {
>
> Should checkpatch.pl care about the spacing after 'for'? Then again,
> it's unrelated to your conversion, so omitting a whitespace cleanup from
> this patch doesn't hurt.
It already does as far as I can tell, but as Richard said, it only
complains about style violations in changed lines.
>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -921,10 +921,8 @@ static void
>> virtio_serial_device_realize(DeviceState *dev, Error **errp)
>> QTAILQ_INIT(&vser->ports);
>>
>> vser->bus.max_nr_ports = vser->serial.max_virtserial_ports;
>> - vser->ivqs = g_malloc(vser->serial.max_virtserial_ports
>> - * sizeof(VirtQueue *));
>> - vser->ovqs = g_malloc(vser->serial.max_virtserial_ports
>> - * sizeof(VirtQueue *));
>> + vser->ivqs = g_new(VirtQueue *, vser->serial.max_virtserial_ports);
>> + vser->ovqs = g_new(VirtQueue *, vser->serial.max_virtserial_ports);
>
> I'm impressed at what Coccinelle can rewrite!
Yup. It's quite capable, but its language is weird, and its error
messages can compete with C++ is mysteriousness (thankfully not in
length).
>> +++ b/ui/keymaps.c
>> @@ -107,7 +107,7 @@ static kbd_layout_t *parse_keyboard_layout(const
>> name2keysym_t *table,
>> }
>>
>> if (!k)
>> - k = g_malloc0(sizeof(kbd_layout_t));
>> + k = g_new0(kbd_layout_t, 1);
>>
>> for(;;) {
>
> Fixing tab damage while you are at it - nice. And just noticed this is
> another instance of no space after 'for', but not worth fixing in this
> patch if checkpatch.pl is happy.
>
> I'm trusting coccinelle, and only glanced through random spots of the
> patch; but where I looked, I didn't find any problems in the conversion.
>
> I don't know if that means we should add:
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thank you!
