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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> 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@redhat.com>
>> ---
>>  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?

* Rename nested local @node to @src_node

* Rename which local variable to @src_bitmap?

* Move nested locals to function scope

> Preexisting, so I'm not insisting that you should do this.
>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 037fa2d435..ffd5cd3b23 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1555,7 +1555,6 @@ bool 
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>      FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>>          const char *name = bdrv_dirty_bitmap_name(bitmap);
>>          uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>> -        Qcow2Bitmap *bm;
>>  
>>          if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
>>              bdrv_dirty_bitmap_inconsistent(bitmap)) {
>> @@ -1625,7 +1624,7 @@ bool 
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>  
>>      /* allocate clusters and store bitmaps */
>>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> -        BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
>> +        bitmap = bm->dirty_bitmap;
>>  
>>          if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
>>              continue;
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!