[PATCH] m68k: Silence -Wshadow=local warnings in the m68k code

Thomas Huth posted 1 patch 7 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230925185603.106945-1-thuth@redhat.com
Maintainers: Laurent Vivier <laurent@vivier.eu>
hw/m68k/bootinfo.h      | 10 ++++------
disas/m68k.c            |  8 ++++----
target/m68k/translate.c |  8 ++++----
3 files changed, 12 insertions(+), 14 deletions(-)
[PATCH] m68k: Silence -Wshadow=local warnings in the m68k code
Posted by Thomas Huth 7 months, 1 week ago
Rename the innermost variables to make the code compile
without warnings when using -Wshadow=local.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/m68k/bootinfo.h      | 10 ++++------
 disas/m68k.c            |  8 ++++----
 target/m68k/translate.c |  8 ++++----
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
index a3d37e3c80..d077d03559 100644
--- a/hw/m68k/bootinfo.h
+++ b/hw/m68k/bootinfo.h
@@ -44,15 +44,14 @@
 
 #define BOOTINFOSTR(base, id, string) \
     do { \
-        int i; \
         stw_p(base, id); \
         base += 2; \
         stw_p(base, \
                  (sizeof(struct bi_record) + strlen(string) + \
                   1 /* null termination */ + 3 /* padding */) & ~3); \
         base += 2; \
-        for (i = 0; string[i]; i++) { \
-            stb_p(base++, string[i]); \
+        for (int _i = 0; string[_i]; _i++) { \
+            stb_p(base++, string[_i]); \
         } \
         stb_p(base++, 0); \
         base = QEMU_ALIGN_PTR_UP(base, 4); \
@@ -60,7 +59,6 @@
 
 #define BOOTINFODATA(base, id, data, len) \
     do { \
-        int i; \
         stw_p(base, id); \
         base += 2; \
         stw_p(base, \
@@ -69,8 +67,8 @@
         base += 2; \
         stw_p(base, len); \
         base += 2; \
-        for (i = 0; i < len; ++i) { \
-            stb_p(base++, data[i]); \
+        for (int _i = 0; _i < len; ++_i) { \
+            stb_p(base++, data[_i]); \
         } \
         base = QEMU_ALIGN_PTR_UP(base, 4); \
     } while (0)
diff --git a/disas/m68k.c b/disas/m68k.c
index aefaecfbd6..a384b4cb64 100644
--- a/disas/m68k.c
+++ b/disas/m68k.c
@@ -1632,10 +1632,10 @@ print_insn_arg (const char *d,
     case '2':
     case '3':
       {
-	int val = fetch_arg (buffer, place, 5, info);
+	int val2 = fetch_arg (buffer, place, 5, info);
         const char *name = 0;
 
-	switch (val)
+	switch (val2)
 	  {
 	  case 2: name = "%tt0"; break;
 	  case 3: name = "%tt1"; break;
@@ -1655,12 +1655,12 @@ print_insn_arg (const char *d,
 	      int break_reg = ((buffer[3] >> 2) & 7);
 
 	      (*info->fprintf_func)
-		(info->stream, val == 0x1c ? "%%bad%d" : "%%bac%d",
+		(info->stream, val2 == 0x1c ? "%%bad%d" : "%%bac%d",
 		 break_reg);
 	    }
 	    break;
 	  default:
-	    (*info->fprintf_func) (info->stream, "<mmu register %d>", val);
+	    (*info->fprintf_func) (info->stream, "<mmu register %d>", val2);
 	  }
 	if (name)
 	  (*info->fprintf_func) (info->stream, "%s", name);
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 9e224fe796..b28d7f7d4b 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -824,14 +824,14 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext *s, int mode, int reg0,
         reg = get_areg(s, reg0);
         result = gen_ldst(s, opsize, reg, val, what, index);
         if (what == EA_STORE || !addrp) {
-            TCGv tmp = tcg_temp_new();
+            TCGv tmp2 = tcg_temp_new();
             if (reg0 == 7 && opsize == OS_BYTE &&
                 m68k_feature(s->env, M68K_FEATURE_M68K)) {
-                tcg_gen_addi_i32(tmp, reg, 2);
+                tcg_gen_addi_i32(tmp2, reg, 2);
             } else {
-                tcg_gen_addi_i32(tmp, reg, opsize_bytes(opsize));
+                tcg_gen_addi_i32(tmp2, reg, opsize_bytes(opsize));
             }
-            delay_set_areg(s, reg0, tmp, true);
+            delay_set_areg(s, reg0, tmp2, true);
         }
         return result;
     case 4: /* Indirect predecrememnt.  */
-- 
2.41.0
Re: [PATCH] m68k: Silence -Wshadow=local warnings in the m68k code
Posted by Markus Armbruster 7 months, 1 week ago
Thomas Huth <thuth@redhat.com> writes:

> Rename the innermost variables to make the code compile
> without warnings when using -Wshadow=local.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Clashes with patches from Philippe and Laurent:

    [PATCH v2 05/22] target/m68k: Clean up local variable shadowing
    [PATCH] disas/m68k: clean up local variable shadowing

You guys figure out how to combine them, please :)
Re: [PATCH] m68k: Silence -Wshadow=local warnings in the m68k code
Posted by Thomas Huth 7 months, 1 week ago
On 26/09/2023 14.19, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> Rename the innermost variables to make the code compile
>> without warnings when using -Wshadow=local.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Clashes with patches from Philippe and Laurent:
> 
>      [PATCH v2 05/22] target/m68k: Clean up local variable shadowing
>      [PATCH] disas/m68k: clean up local variable shadowing
> 
> You guys figure out how to combine them, please :)

Ok, then never mind about my patch.
Anyway, it's getting confusing what has already been fixed and what not ... 
could you please pick up all patches that are available so far and send a 
pull request for them?

  Thanks,
   Thomas
Re: [PATCH] m68k: Silence -Wshadow=local warnings in the m68k code
Posted by Markus Armbruster 7 months, 1 week ago
Thomas Huth <thuth@redhat.com> writes:

> On 26/09/2023 14.19, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> Rename the innermost variables to make the code compile
>>> without warnings when using -Wshadow=local.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>
>> Clashes with patches from Philippe and Laurent:
>>
>>      [PATCH v2 05/22] target/m68k: Clean up local variable shadowing
>>      [PATCH] disas/m68k: clean up local variable shadowing
>>
>> You guys figure out how to combine them, please :)
>
> Ok, then never mind about my patch.
> Anyway, it's getting confusing what has already been fixed and what not ... could you please pick up all patches that are available so far and send a pull request for them?

I'm collecting at https://repo.or.cz/qemu/armbru.git in branch
shadow-next.  I'll send a new summary shortly.  I intend to send a PR,
but first I need to collect R-bys and drop patches that aren't ready, if
any.
Re: [PATCH] m68k: Silence -Wshadow=local warnings in the m68k code
Posted by Laurent Vivier 7 months, 1 week ago
Le 25/09/2023 à 20:56, Thomas Huth a écrit :
> Rename the innermost variables to make the code compile
> without warnings when using -Wshadow=local.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/m68k/bootinfo.h      | 10 ++++------
>   disas/m68k.c            |  8 ++++----
>   target/m68k/translate.c |  8 ++++----
>   3 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
> index a3d37e3c80..d077d03559 100644
> --- a/hw/m68k/bootinfo.h
> +++ b/hw/m68k/bootinfo.h
> @@ -44,15 +44,14 @@
>   
>   #define BOOTINFOSTR(base, id, string) \
>       do { \
> -        int i; \
>           stw_p(base, id); \
>           base += 2; \
>           stw_p(base, \
>                    (sizeof(struct bi_record) + strlen(string) + \
>                     1 /* null termination */ + 3 /* padding */) & ~3); \
>           base += 2; \
> -        for (i = 0; string[i]; i++) { \
> -            stb_p(base++, string[i]); \
> +        for (int _i = 0; string[_i]; _i++) { \
> +            stb_p(base++, string[_i]); \
>           } \
>           stb_p(base++, 0); \
>           base = QEMU_ALIGN_PTR_UP(base, 4); \
> @@ -60,7 +59,6 @@
>   
>   #define BOOTINFODATA(base, id, data, len) \
>       do { \
> -        int i; \
>           stw_p(base, id); \
>           base += 2; \
>           stw_p(base, \
> @@ -69,8 +67,8 @@
>           base += 2; \
>           stw_p(base, len); \
>           base += 2; \
> -        for (i = 0; i < len; ++i) { \
> -            stb_p(base++, data[i]); \
> +        for (int _i = 0; _i < len; ++_i) { \
> +            stb_p(base++, data[_i]); \
>           } \
>           base = QEMU_ALIGN_PTR_UP(base, 4); \
>       } while (0)
> diff --git a/disas/m68k.c b/disas/m68k.c
> index aefaecfbd6..a384b4cb64 100644
> --- a/disas/m68k.c
> +++ b/disas/m68k.c
> @@ -1632,10 +1632,10 @@ print_insn_arg (const char *d,
>       case '2':
>       case '3':
>         {
> -	int val = fetch_arg (buffer, place, 5, info);
> +	int val2 = fetch_arg (buffer, place, 5, info);
>           const char *name = 0;
>   
> -	switch (val)
> +	switch (val2)
>   	  {
>   	  case 2: name = "%tt0"; break;
>   	  case 3: name = "%tt1"; break;
> @@ -1655,12 +1655,12 @@ print_insn_arg (const char *d,
>   	      int break_reg = ((buffer[3] >> 2) & 7);
>   
>   	      (*info->fprintf_func)
> -		(info->stream, val == 0x1c ? "%%bad%d" : "%%bac%d",
> +		(info->stream, val2 == 0x1c ? "%%bad%d" : "%%bac%d",
>   		 break_reg);
>   	    }
>   	    break;
>   	  default:
> -	    (*info->fprintf_func) (info->stream, "<mmu register %d>", val);
> +	    (*info->fprintf_func) (info->stream, "<mmu register %d>", val2);
>   	  }
>   	if (name)
>   	  (*info->fprintf_func) (info->stream, "%s", name);

"reg" would be a better name than "val2".

> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 9e224fe796..b28d7f7d4b 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -824,14 +824,14 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext *s, int mode, int reg0,
>           reg = get_areg(s, reg0);
>           result = gen_ldst(s, opsize, reg, val, what, index);
>           if (what == EA_STORE || !addrp) {
> -            TCGv tmp = tcg_temp_new();
> +            TCGv tmp2 = tcg_temp_new();
>               if (reg0 == 7 && opsize == OS_BYTE &&
>                   m68k_feature(s->env, M68K_FEATURE_M68K)) {
> -                tcg_gen_addi_i32(tmp, reg, 2);
> +                tcg_gen_addi_i32(tmp2, reg, 2);
>               } else {
> -                tcg_gen_addi_i32(tmp, reg, opsize_bytes(opsize));
> +                tcg_gen_addi_i32(tmp2, reg, opsize_bytes(opsize));
>               }
> -            delay_set_areg(s, reg0, tmp, true);
> +            delay_set_areg(s, reg0, tmp2, true);
>           }
>           return result;
>       case 4: /* Indirect predecrememnt.  */

"inc" would be a better name than "val2".

Otherwise:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>


Thanks,
Laurent