diff mbox series

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

Message ID 20230831132546.3525721-5-armbru@redhat.com (mailing list archive)
State Superseded
Headers show
Series [1/7] migration/rdma: Fix save_page method to fail on polling error | expand

Commit Message

Markus Armbruster Aug. 31, 2023, 1:25 p.m. UTC
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(-)

Comments

Stefan Hajnoczi Aug. 31, 2023, 7:29 p.m. UTC | #1
On Thu, Aug 31, 2023 at 03:25:43PM +0200, Markus Armbruster wrote:
> 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(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Kevin Wolf Sept. 15, 2023, 7:52 a.m. UTC | #2
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).

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>
Markus Armbruster Sept. 19, 2023, 5:48 a.m. UTC | #3
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!
Kevin Wolf Sept. 19, 2023, 9:45 a.m. UTC | #4
Am 19.09.2023 um 07:48 hat Markus Armbruster geschrieben:
> 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?

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.

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

Kevin
Markus Armbruster Sept. 20, 2023, 1:38 p.m. UTC | #5
Kevin Wolf <kwolf@redhat.com> writes:

> Am 19.09.2023 um 07:48 hat Markus Armbruster geschrieben:
>> 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?
>
> 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!
diff mbox series

Patch

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);
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;