qemu-trivial
[Top][All Lists]
Advanced

[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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]