[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH 01/88] cocci: script to use g_new
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH 01/88] cocci: script to use g_new() & friends |
Date: |
Thu, 2 Nov 2017 01:16:32 -0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
Hi Markus,
On 10/09/2017 04:24 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
>
>> Imported from Markus Armbruster commit b45c03f
>>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>> Signed-off-by: Markus Armbruster <address@hidden>?
>>
>> scripts/coccinelle/g_new.cocci | 101
>> +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 101 insertions(+)
>> create mode 100644 scripts/coccinelle/g_new.cocci
>>
>> diff --git a/scripts/coccinelle/g_new.cocci b/scripts/coccinelle/g_new.cocci
>> new file mode 100644
>> index 0000000000..1e57685a6b
>> --- /dev/null
>> +++ b/scripts/coccinelle/g_new.cocci
>> @@ -0,0 +1,101 @@
>> +/* transform g_new*() alloc with size arguments of the form sizeof(T) [* N]
>
> Confusing. Sounds like we transform g_new() into something else. Also,
> wing both ends of the comment, and start with a capital letter.
Ok
>
>> + *
>> + * g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
>> + * for two reasons. One, it catches multiplication overflowing size_t.
>> + * two, it returns T * rather than void *, which lets the compiler catch
>
> Start your sentences with a capital letter. Or rather, keep my commit
> message's capital letters intact ;)
Yes :|
>
>> + * more type errors.
>> + *
>> + * Copyright: (C) 2017 Markus Armbruster <address@hidden>. GPLv2+.
>
> If you want to put a copyright note into this file, you should stick to
> the common format:
>
> /*
> * Rewrite memory allocations to use g_new() & friends
> *
> * Copyright (C) 2014-2017 Red Hat, Inc.
> *
> * Authors:
> * Markus Armbruster <address@hidden>,
> *
> * This work is licensed under the terms of the GNU GPL, version 2 or later.
> * See the COPYING file in the top-level directory.
> */
Ok
>> + * (Imported from b45c03f585ea9bb1af76c73e82195418c294919d)
>
> Scratch this line. It's in the commit message, and that suffices.
Ok
>> + *
>> + * See
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg00908.html:
>> + *
>> + * g_new() advantages (from glib doc):
>> + * - the returned pointer is cast to a pointer to the given type.
>> + * - care is taken to avoid overflow when calculating the size of the
>> + * allocated block.
>
> Repeats what you just said in the text pasted from my commit message.
Ok
>> + *
>> + * p = g_malloc(sizeof(*p)) is idiomatic, and not obviously inferior to
>> + * p = g_new(T, 1), where T is the type of *p.
>> + * But once you add multiplication, g_new() adds something useful:
>> overflow
>> + * protection. Conversion to g_new() might make sense then.
>> + */
>
> This part doesn't really apply, yet: the semantic patch doesn't deal
> with sizeof(*p).
>
> Please write a coherent introduction instead of a series of quotations.
Sorry :S I'll try :)
>> +
>> +@@
>> +type T;
>> +@@
>> +-g_malloc(sizeof(T))
>> ++g_new(T, 1)
>> +@@
>> +type T;
>> +@@
>> +-g_try_malloc(sizeof(T))
>> ++g_try_new(T, 1)
>> +@@
>> +type T;
>> +@@
>> +-g_malloc0(sizeof(T))
>> ++g_new0(T, 1)
>> +@@
>> +type T;
>> +@@
>> +-g_try_malloc0(sizeof(T))
>> ++g_try_new0(T, 1)
>> +
>
> Any particular reason for adding this blank line?
>
> You add more below, and a comment. Okay, but I'd commit the script
> exactly as given in commit b45c03f, then add improvements on top.
Yes.
>> +@@
>> +type T;
>> +expression n;
>> +@@
>> +-g_malloc(sizeof(T) * (n))
>> ++g_new(T, n)
>> +@@
>> +type T;
>> +expression n;
>> +@@
>> +-g_try_malloc(sizeof(T) * (n))
>> ++g_try_new(T, n)
>> +@@
>> +type T;
>> +expression n;
>> +@@
>> +-g_malloc0(sizeof(T) * (n))
>> ++g_new0(T, n)
>> +@@
>> +type T;
>> +expression n;
>> +@@
>> +-g_try_malloc0(sizeof(T) * (n))
>> ++g_try_new0(T, n)
>> +
>> +@@
>> +type T;
>> +expression p, n;
>> +@@
>> +-g_realloc(p, sizeof(T) * (n))
>> ++g_renew(T, p, n)
>> +@@
>> +type T;
>> +expression p, n;
>> +@@
>> +-g_try_realloc(p, sizeof(T) * (n))
>> ++g_try_renew(T, p, n)
>> +
>> +// drop superfluous cast
>> +@@
>> +type T;
>> +expression n;
>> +@@
>> +-(T *)g_new(T, n)
>> ++g_new(T, n)
>> +@@
>> +type T;
>> +expression n;
>> +@@
>> +-(T *)g_new0(T, n)
>> ++g_new0(T, n)
>> +@@
>> +type T;
>> +expression p, n;
>> +@@
>> +-(T *)g_renew(T, p, n)
>> ++g_renew(T, p, n)
Thanks for your review :)
Phil.
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-trivial] [Qemu-devel] [PATCH 01/88] cocci: script to use g_new() & friends,
Philippe Mathieu-Daudé <=