Re: [PATCH] Fix -Wshadow=local warnings in genmatch.c

[Richard Biener]

On Thu, Oct 3, 2019 at 5:18 PM Bernd Edlinger <bernd.edlin...@hotmail.de> wrote:
>
> Hi,
>
> this fixes -Wshadow=local warnings in genmatch.c itself and in generated
> files as well.
>
> The non-trivial part of this patch is renaming the generated local variables
> in the gimple-match.c, to use different names for variables in inner blocks,
> and use a depth index to access the right value.  Also this uses variables
> in the reserved name space, to avoid using the same names (e.g. res, op0, op1)
> that are used in existing .md files.
>
> So I rename:
>
> ops%d -> _o%d
> op%d -> _p%d
> o%u%u -> _q%u%u
> res -> _r%d (in gen_transform)
> res -> _r (in dt_simplify::gen_1)
> def -> _a%d (if gimple_assign)
> def -> _c%d (if gimple_call)
> def_stmt -> _d%d
>
> leaving res_ops, res_op, capture for later, since they are not likely to
> be used in .md files.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

Jumping on one set of hunks:

@@ -2270,42 +2270,42 @@ capture_info::walk_result (operand *o, bool condit
          walk_result (e->ops[i], cond_p, result);
        }
     }
-  else if (if_expr *e = dyn_cast <if_expr *> (o))
+  else if (if_expr *e1 = dyn_cast <if_expr *> (o))
     {
...
+  else if (with_expr *e2 = dyn_cast <with_expr *> (o))
     {
-      bool res = (e->subexpr == result);
..

this seems like a bogus -Wshadow if it really warns?  The change above makes
the code ugly IMHO.  Indeed:

> g++-8 t.C -Wshadow=local
t.C: In function ‘void foo(int)’:
t.C:5:16: warning: declaration of ‘j’ shadows a previous local [-Wshadow=local]
   else if (int j = 1)
                ^
t.C:3:11: note: shadowed declaration is here
   if (int j = i)
           ^

for

void foo (int i)
{
  if (int j = i)
    ;
  else if (int j = 1)
    ;
}

and that's a bug.  Ugh.

void foo (int i)
{
  if (int j = i)
    ;
  else
    j = 1;
}

really compiles :/  I wasn't aware of that semantic and it's totally
non-obvious to me... ICK.  Btw, rather than e1,e2,e3... I'd
then have used e (expr *), ie (if_expr *) and we (with_expr).
1 2 and 3 are so arbitrary.

The rest of the patch looks OK to me, the above just caught my eye
so OK for trunk with e, ie, we.

Thanks,
Richard.

>
>
> Thanks
> Bernd.
>