On 10/4/19 12:54 PM, Richard Biener wrote:
> 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.
>
Admittedly I was also completely surprised that the if scope
extends to the else block. If that is in fact wrong, we ought to
fix that before I ruin the while code-base with changes like that.
> The rest of the patch looks OK to me, the above just caught my eye
> so OK for trunk with e, ie, we.
>
Okay will change e1 to ie (if_expr), e2 to we (with_expr)
and e3 to ce (c_expr) before committing.
Thanks
Bernd.
