On 10/09/2017 03:11 AM, Dr. David Alan Gilbert wrote: >>> >>> - info = g_malloc0(sizeof(*info)); >>> + info = g_new0(CommandInfoList, 1); >>> info->value = g_malloc0(sizeof(*info->value)); >>> info->value->name = g_strdup(cmd->name); >>> info->next = *list; >> >> I'm not convinced rewriting >> >> LHS = g_malloc(sizeof(*LHS)); >> >> to >> >> LHS = g_new(T, 1); >> >> where T is the type of LHS is worth the trouble. The code before the >> rewrite is pretty idiomatic. There's no possibility of integer overflow >> g_new() could avoid. The types are obviously correct, so the additional >> type checking is quite unlikely to catch anything. That leaves the >> consistency argument. I'm willing to hear it, but I feel it should be >> heard in a patch series that does nothing else. > > The 'obviously correct' is the dodgy part of the argument here. > How many bugs do we right that are obviously wrong? > > t.c:13:20: warning: initialization from incompatible pointer type > [-Wincompatible-pointer-types] > struct c *pc = g_new(struct b, 1); > > seems good to me.
Yes, that's a GOOD warning, and it proves that we DO get type safety when we convert: LHS = g_malloc(sizeof(type)) into LHS = g_new(type, 1) but that's not what Markus was pointing out. When we already have the correctly-typed object on the right hand side, as in: LHS = g_malloc(sizeof(*LHS)) then the compiler will always give us the correct type of LHS, whereas with: LHS = g_new(type, 1) we have to manually update the line if the type of LHS changes. Thus, converting malloc(sizeof(type)) into new(type, 1) is a no-brainer, but converting malloc(sizeof(*expr)) needs justification. I'm not necessarily opposed to the conversion (if our justification is consistency, and where HACKING documents our style and where we have scripts that let us easily preserve our style), but I'm not sure I see the requisite justification in the current iteration of this series. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
