[Libguestfs] [PATCH nbdkit 4/9] common/regions: Use new vector type to store the list of regions.
Eric Blake
eblake at redhat.com
Wed Apr 15 20:28:24 UTC 2020
On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> A fairly straightforward replacement, but note that we must rename all
> variables called ‘regions’ as something else (eg. ‘rs’) because
> -Wshadow warns about them (which is surprising to me since I thought
> this warning only applied to local vs global variable, not local
> variable vs global typedef).
>
> Also I got rid of the get_regions accessor method, replacing it
> everywhere with direct use of regions->ptr[]. Although the accessor
> method did bounds checking, my reasoning is this change is correct
> because in all cases we were iterating over the regions from
> 0 .. nr_regions-1. However an improvement would be to have a proper
> iterator, although that change is not so straightforward because of
> lack of closures in C.
> ---
> @@ -70,38 +72,35 @@ struct region {
> const char *description;
> };
>
> -/* Array of regions. */
> -struct regions {
> - struct region *regions;
> - size_t nr_regions;
> -};
> +/* Vector of struct region. */
> +DEFINE_VECTOR_TYPE(regions, struct region);
>
> -extern void init_regions (struct regions *regions)
> +extern void init_regions (regions *regions)
> __attribute__((__nonnull__ (1)));
This change makes sense (DEFINE_VECTOR_TYPE gives us a typedef, so you
lose the explicit 'struct').
> -extern void free_regions (struct regions *regions)
> +extern void free_regions (regions *regions)
> __attribute__((__nonnull__ (1)));
>
> /* Return the number of regions. */
> static inline size_t __attribute__((__nonnull__ (1)))
> -nr_regions (struct regions *regions)
> +nr_regions (struct regions *rs)
whereas this one is odd. Did you mean to drop 'struct' here? If so, do
you still have to rename the variable to rs? Okay, I'm seeing the
pattern - forward declarations don't trigger -Wshadow, but
implementations do.
> {
> - return regions->nr_regions;
> + return rs->size;
> }
>
> /* Return the virtual size of the disk. */
> static inline int64_t __attribute__((__nonnull__ (1)))
> -virtual_size (struct regions *regions)
> +virtual_size (regions *rs)
here, you both dropped 'struct' and did the rename; the rename because
it is the implementation.
> +++ b/common/regions/regions.c
Overall, the rest of the patch is reasonable (mostly mechanical due to
the renames).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
More information about the Libguestfs
mailing list