From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkSjK-0000S8-Df for qemu-devel@nongnu.org; Fri, 09 Oct 2015 04:08:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkSjH-0008SH-59 for qemu-devel@nongnu.org; Fri, 09 Oct 2015 04:08:42 -0400 References: <1444332916-16476-1-git-send-email-thuth@redhat.com> <1444332916-16476-5-git-send-email-thuth@redhat.com> <5616D0FF.5030909@redhat.com> <87fv1k1qu8.fsf@blackfin.pond.sub.org> From: Laszlo Ersek Message-ID: <56177603.40307@redhat.com> Date: Fri, 9 Oct 2015 10:08:35 +0200 MIME-Version: 1.0 In-Reply-To: <87fv1k1qu8.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] tests/i44fx-test: No need for zeroing memory before memset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-trivial@nongnu.org, Thomas Huth , qemu-devel@nongnu.org On 10/09/15 08:46, Markus Armbruster wrote: > Laszlo Ersek 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 >>> Signed-off-by: Thomas Huth >>> --- >>> 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 Cheers Laszlo