Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense

[Alex Bennée <alex.bennee@xxxxxxxxxx>]

Markus Armbruster <armbru@xxxxxxxxxx> writes:

> 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).
>
> Patch created mechanically with:
>
>     $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>            --macro-file scripts/cocci-macro-file.h FILES...
>
> Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
<snip>
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -97,9 +97,9 @@ static void qjack_buffer_create(QJackBuffer *buffer, int 
> channels, int frames)
>      buffer->used     = 0;
>      buffer->rptr     = 0;
>      buffer->wptr     = 0;
> -    buffer->data     = g_malloc(channels * sizeof(float *));
> +    buffer->data     = g_new(float *, channels);
>      for (int i = 0; i < channels; ++i) {
> -        buffer->data[i] = g_malloc(frames * sizeof(float));
> +        buffer->data[i] = g_new(float, frames);

Are these actually buffers of pointers to floats? I guess I leave that
to the JACK experts...

>      }
>  }
>  
> @@ -453,7 +453,7 @@ static int qjack_client_init(QJackClient *c)
>      jack_on_shutdown(c->client, qjack_shutdown, c);
>  
>      /* allocate and register the ports */
> -    c->port = g_malloc(sizeof(jack_port_t *) * c->nchannels);
> +    c->port = g_new(jack_port_t *, c->nchannels);
>      for (int i = 0; i < c->nchannels; ++i) {

I guess JACK just likes double indirection...

>  
>          char port_name[16];
<snip>
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -177,7 +177,7 @@ static void register_vfs(PCIDevice *dev)
>      assert(sriov_cap > 0);
>      num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
>  
> -    dev->exp.sriov_pf.vf = g_malloc(sizeof(PCIDevice *) * num_vfs);
> +    dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
>      assert(dev->exp.sriov_pf.vf);

So what I find confusing about the conversion of sizeof(foo *) is that
while the internal sizeof in g_new is unaffected I think the casting
ends up as

 foo **

but I guess the compiler would have complained so maybe I don't
understand the magic enough.

<snip>
> index 42130667a7..598e6adc5e 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
> @@ -41,7 +41,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const char *name, 
> PCIDevice *dev,
>      qatomic_set(&ring->ring_state->cons_head, 0);
>      */
>      ring->npages = npages;
> -    ring->pages = g_malloc0(npages * sizeof(void *));
> +    ring->pages = g_new0(void *, npages);

At least here ring->pages agrees about void ** being the result.

<snip>

So other than my queries about the sizeof(foo *) which I'd like someone
to assuage me of my concerns it looks like the script has done a
thorough mechanical job as all good scripts should ;-)

Reviewed-by: Alex Bennée <alex.bennee@xxxxxxxxxx>

-- 
Alex Bennée