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
_______________________________________________
Libguestfs mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libguestfs
