Re: [RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing

[Daniel P . Berrangé]

On Mon, Sep 04, 2023 at 06:12:34PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   softmmu/physmem.c: In function 
> ‘cpu_physical_memory_snapshot_and_clear_dirty’:
>   softmmu/physmem.c:916:27: warning: declaration of ‘offset’ shadows a 
> parameter [-Wshadow=compatible-local]
>     916 |             unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
>         |                           ^~~~~~
>   softmmu/physmem.c:892:31: note: shadowed declaration is here
>     892 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned 
> client)
>         |                        ~~~~~~~^~~~~~
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Please double-check how 'offset' is used few lines later.

I don't see an issue - those lines are in an outer scope, so won't
be accessing the 'offset' you've changed, they'll be the parameter
instead. If you want to sanity check though, presumably the asm
dissassembly for this method should be the same before/after this
change

> ---
>  softmmu/physmem.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 18277ddd67..db5b628a60 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -913,16 +913,16 @@ DirtyBitmapSnapshot 
> *cpu_physical_memory_snapshot_and_clear_dirty
>  
>          while (page < end) {
>              unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> -            unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +            unsigned long ofs = page % DIRTY_MEMORY_BLOCK_SIZE;
>              unsigned long num = MIN(end - page,
> -                                    DIRTY_MEMORY_BLOCK_SIZE - offset);
> +                                    DIRTY_MEMORY_BLOCK_SIZE - ofs);
>  
> -            assert(QEMU_IS_ALIGNED(offset, (1 << BITS_PER_LEVEL)));
> +            assert(QEMU_IS_ALIGNED(ofs, (1 << BITS_PER_LEVEL)));
>              assert(QEMU_IS_ALIGNED(num,    (1 << BITS_PER_LEVEL)));
> -            offset >>= BITS_PER_LEVEL;
> +            ofs >>= BITS_PER_LEVEL;
>  
>              bitmap_copy_and_clear_atomic(snap->dirty + dest,
> -                                         blocks->blocks[idx] + offset,
> +                                         blocks->blocks[idx] + ofs,
>                                           num);
>              page += num;
>              dest += num >> BITS_PER_LEVEL;
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|