Re: [Qemu-trivial] [Qemu-devel] clang-tidy: use g_new() family of functions

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi,
>
> I have a series of changes generated with clang-tidy qemu [1] pending
> for review [2].
>
> It translates calloc/*malloc*/*realloc() calls to
> g_new/g_newa/g_new0/g_renew() where the argument is a sizeof(T) [* N].

Only for *type* T, or for arbitrary T?

For type T, we've done the transformation repeatedly with Coccinelle,
first in commit b45c03f (commit message appended).  Repeatedly, because
they creep back in.

How does your technique compare to this prior one?

> This is the first commit to give you an idea:
>
> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> index 98fddd7ac1..75affcb8a6 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -166,7 +166,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
>      arm_timer_state *s;
>      QEMUBH *bh;
>
> -    s = (arm_timer_state *)g_malloc0(sizeof(arm_timer_state));
> +    s = g_new0(arm_timer_state, 1);
>      s->freq = freq;
>      s->control = TIMER_CTRL_IE;
>
> 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.
>
> But it is also shorter&nicer :)
>
> I have not included in the first batch the opportunity to also
> translate: alloc(sizeof(*p) * x) to g_new(typeof(*p), x), since it is
> arguably not much nicer. But for consistency, I think it would be good
> to use g_new(). What do you think?

p = g_malloc(sizeof(*p)) is idiomatic, and not obviously inferior to
p = g_new(T, 1), where T is the type of *p.  Use whatever is easier to
read, I guess.

But once you add multiplication, g_new() adds something useful: overflow
protection.  Conversion to g_new() might make sense then.

> I splitted the changes amont the various MAINTAINERS entries, but it
> is still about 70 patches (without the typeof() changes).
>
>
> [1] https://github.com/elmarco/clang-tools-extra/
> [2] https://github.com/elmarco/qemu/commits/gnew


commit b45c03f585ea9bb1af76c73e82195418c294919d
Author: Markus Armbruster <address@hidden>
Date:   Mon Sep 7 10:39:27 2015 +0100

    arm: Use g_new() & friends where that makes obvious sense
    
    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.
    
    This commit only touches allocations with size arguments of the form
    sizeof(T).
    
    Coccinelle semantic patch:
    
        @@
        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)
        @@
        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)
        @@
        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)
    
    Signed-off-by: Markus Armbruster <address@hidden>
    Reviewed-by: Eric Blake <address@hidden>
    Message-id: address@hidden
    Signed-off-by: Peter Maydell <address@hidden>