Hi Markus, On 10/09/2017 04:24 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <f4...@amsat.org> writes: > >> Imported from Markus Armbruster commit b45c03f >> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> >> --- >> Signed-off-by: Markus Armbruster <arm...@redhat.com>? >> >> 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 <arm...@redhat.com>. 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 <arm...@redhat.com>, > * > * 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.
