| Message ID | 1444332916-16476-5-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/10/15 22:24, Laszlo Ersek wrote: > 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 <lersek@redhat.com> >> Signed-off-by: Thomas Huth <thuth@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? g_new IMHO only makes sense when you try to allocate the memory for a struct or something similar - for allocating byte arrays, g_malloc is the better choice. So I think this patch should be fine. Thomas
Laszlo Ersek <lersek@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 <lersek@redhat.com> >> Signed-off-by: Thomas Huth <thuth@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.
On 10/09/15 08:46, Markus Armbruster wrote: > Laszlo Ersek <lersek@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 <lersek@redhat.com> >>> Signed-off-by: Thomas Huth <thuth@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 <lersek@redhat.com> Cheers Laszlo
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);
Change a g_malloc0 into g_malloc since the following memset fills the whole buffer anyway. Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> --- tests/i440fx-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)