On 10/09/15 08:46, Markus Armbruster wrote: > Laszlo Ersek <ler...@redhat.com> writes: > >> On 10/08/15 21:35, Thomas Huth wrote: >>> Change a g_malloc0 into g_malloc since the following >>> memset fills the whole buffer anyway. >>> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Signed-off-by: Thomas Huth <th...@redhat.com> >>> --- >>> tests/i440fx-test.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c >>> index d0bc8de..7fa1709 100644 >>> --- a/tests/i440fx-test.c >>> +++ b/tests/i440fx-test.c >>> @@ -191,7 +191,7 @@ static void write_area(uint32_t start, uint32_t end, >>> uint8_t value) >>> uint32_t size = end - start + 1; >>> uint8_t *data; >>> >>> - data = g_malloc0(size); >>> + data = g_malloc(size); >>> memset(data, value, size); >>> memwrite(start, data, size); >>> >>> >> >> Technically you are right of course, but I remember some historical mess >> around this, in this file. >> >> Plus I vaguely recall g_new[0]() being the most recent preference. >> >> https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-new >> >> See e.g. commit 97f3ad3551. Markus? > > TL;DR: the patch is fine. > > The argument of g_malloc() isn't always the size of a type. But when it > is, you should be using g_new() instead. The commit message explains: > > 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 > more type errors. > > The multiplication argument applies only when n != 1. > > The type checking argument applies regardless of n. That's why the > commit also transfroms patterns like g_malloc(sizeof(T)). > > When the argument isn't written as sizeof a type, but could be, we enter > "matter of taste" territory. For instance, some may perefer the > idiomatic T *p = g_malloc(sizeof(*p)) to the more tightly typed > p = g_new(T, 1). I therefore leave them alone. Commit messsage again: > > This commit only touches allocations with size arguments of the form > sizeof(T). > > A separate issue is a zeroing memset() after allocation. Please use a > zeroing allocator instead, because that's more concise. Precedence: > commit 0bd0adb. > > Now let's apply this to the patch. > > The memset() isn't zeroing, so "use a zeroing allocator instead" doesn't > apply. Before the patch, the code uses one additionally, which is > wasteful. The patch stops the waste. > > The size argument is not the size of any type. You could still write > > data = g_new(uint8_t, size); > > but the extra verbosity pretty clearly outweighs what we could gain from > type checking here. >
Okay, thanks! Reviewed-by: Laszlo Ersek <ler...@redhat.com> Cheers Laszlo
