Re: [PATCH 4/7] block/dirty-bitmap: Clean up local variable shadowing

[Markus Armbruster <armbru@xxxxxxxxxx>]

Kevin Wolf <kwolf@xxxxxxxxxx> writes:

> Am 19.09.2023 um 07:48 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@xxxxxxxxxx> writes:
>> 
>> > Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
>> >> Local variables shadowing other local variables or parameters make the
>> >> code needlessly hard to understand.  Tracked down with -Wshadow=local.
>> >> Clean up: delete inner declarations when they are actually redundant,
>> >> else rename variables.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
>> >> ---
>> >>  block/monitor/bitmap-qmp-cmds.c | 2 +-
>> >>  block/qcow2-bitmap.c            | 3 +--
>> >>  2 files changed, 2 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/block/monitor/bitmap-qmp-cmds.c 
>> >> b/block/monitor/bitmap-qmp-cmds.c
>> >> index 55f778f5af..4d018423d8 100644
>> >> --- a/block/monitor/bitmap-qmp-cmds.c
>> >> +++ b/block/monitor/bitmap-qmp-cmds.c
>> >> @@ -276,7 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char 
>> >> *node, const char *target,
>> >>  
>> >>      for (lst = bms; lst; lst = lst->next) {
>> >>          switch (lst->value->type) {
>> >> -            const char *name, *node;
>> >> +            const char *name;
>> >>          case QTYPE_QSTRING:
>> >>              name = lst->value->u.local;
>> >>              src = bdrv_find_dirty_bitmap(bs, name);
>> >
>> > The names in this function are all over the place... A more ambitious
>> > patch could rename the parameters to dst_node/dst_bitmap and these
>> > variables to src_node/src_bitmap to get some more consistency (both with
>> > each other and with the existing src/dst variables).
>> 
>> What exactly would you like me to consider?  Perhaps:
>> 
>> * Rename parameter @node to @dst_node
>> 
>> * Rename which parameter to @dst_bitmap?
>
> Parameter @target to @dst_bitmap (it's the name of a bitmap in
> @node/@dst_node)
>
>> * Rename nested local @node to @src_node
>> 
>> * Rename which local variable to @src_bitmap?
>
> @name to @src_bitmap (it's the name of a bitmap in the local
> @node/@src_node)
>
>> * Move nested locals to function scope
>
> I don't really mind either way, but yes, maybe that would be more
> conventional.

Done for v2.

> That you couldn't tell for two of the variables what they actually are
> probably supports the argument that they should be renamed. :-)

Fair point!