Re: [PATCH v2 18/22] semihosting/arm-compat: Clean up local variable shadowing

[Peter Maydell]

On Mon, 4 Sept 2023 at 17:15, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Fix:
>
>   semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’:
>   semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows 
> a previous local [-Wshadow=local]
>     379 |         int ret, err = 0;
>         |             ^~~
>   semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here
>     370 |     uint32_t ret;
>         |              ^~~
>   semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows 
> a previous local [-Wshadow=local]
>     682 |                 abi_ulong ret;
>         |                           ^~~
>   semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here
>     370 |     int ret;
>         |         ^~~
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  semihosting/arm-compat-semi.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

If I'm reading the code correctly, the top level 'ret' variable
is only used by the SYS_EXIT case currently. So rather than
changing the type of it I think it would be better to remove
it and have a variable at tighter scope for SYS_EXIT. I
think that's easier to read than this kind of "single variable
used for multiple purposes at different places within a
long function".

> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 564fe17f75..85852a15b8 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs)
>      target_ulong ul_ret;
>      char * s;
>      int nr;
> -    uint32_t ret;
> +    int ret;
>      int64_t elapsed;
>
>      nr = common_semi_arg(cs, 0) & 0xffffffffU;
> @@ -376,7 +376,7 @@ void do_common_semihosting(CPUState *cs)
>      switch (nr) {
>      case TARGET_SYS_OPEN:
>      {
> -        int ret, err = 0;
> +        int err = 0;
>          int hostfd;
>
>          GET_ARG(0);
> @@ -679,14 +679,11 @@ void do_common_semihosting(CPUState *cs)
>               * allocate it using sbrk.
>               */
>              if (!ts->heap_limit) {
> -                abi_ulong ret;
> -
>                  ts->heap_base = do_brk(0);
>                  limit = ts->heap_base + COMMON_SEMI_HEAP_SIZE;
>                  /* Try a big heap, and reduce the size if that fails.  */
>                  for (;;) {
> -                    ret = do_brk(limit);
> -                    if (ret >= limit) {
> +                    if (do_brk(limit) >= limit) {
>                          break;
>                      }
>                      limit = (ts->heap_base >> 1) + (limit >> 1);
> -

thanks
-- PMM