[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/88] cocci: script to use g_new() & friends
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 01/88] cocci: script to use g_new() & friends |
Date: |
Mon, 09 Oct 2017 09:24:16 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
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.
> + *
> + * 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 ;)
> + * 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.
*/
> + * (Imported from b45c03f585ea9bb1af76c73e82195418c294919d)
Scratch this line. It's in the commit message, and that suffices.
> + *
> + * 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.
> + *
> + * 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.
> +
> +@@
> +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.
> +@@
> +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)
- [Qemu-devel] [PATCH 00/88] use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 01/88] cocci: script to use g_new() & friends, Philippe Mathieu-Daudé, 2017/10/06
- Re: [Qemu-devel] [PATCH 01/88] cocci: script to use g_new() & friends,
Markus Armbruster <=
- [Qemu-devel] [PATCH 02/88] cocci: add more g_new() transformations, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 03/88] cocci: extract typeof() from g_new(), Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 04/88] cocci: avoid use of g_new0(), Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 05/88] cocci: use g_strfreev(), Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 06/88] ARM: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 07/88] Audio: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 08/88] BT: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06
- [Qemu-devel] [PATCH 09/88] Bootdevice: use g_new() family of functions, Philippe Mathieu-Daudé, 2017/10/06