Re: [PATCH] Fix -Wshadow=local warnings in genmatch.c
On Fri, Oct 04, 2019 at 11:52:11AM +, Bernd Edlinger wrote: > 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. It is correct. http://eel.is/c++draft/stmt.stmt [stmt.stmt]/5 says: "A name introduced in a selection-statement or iteration-statement outside of any substatement is in scope from its point of declaration until the end of the statement's substatements. Such a name cannot be redeclared in the outermost block of any of the substatements." if with else has 2 substatements and the condition declaration is in scope in both of them. Jakub
Re: [PATCH] Fix -Wshadow=local warnings in genmatch.c
On 10/4/19 12:54 PM, Richard Biener wrote:
> On Thu, Oct 3, 2019 at 5:18 PM Bernd Edlinger
> 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 (o))
> + else if (if_expr *e1 = dyn_cast (o))
> {
> ...
> + else if (with_expr *e2 = dyn_cast (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.
Re: [PATCH] Fix -Wshadow=local warnings in genmatch.c
On Thu, Oct 3, 2019 at 5:18 PM Bernd Edlinger 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 (o))
+ else if (if_expr *e1 = dyn_cast (o))
{
...
+ else if (with_expr *e2 = dyn_cast (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.
>
[PATCH] Fix -Wshadow=local warnings in genmatch.c
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?
Thanks
Bernd.
2019-10-03 Bernd Edlinger
* genmatch.c (commutate): Rename local var.
(lower_cond): Reuse local var.
(dt_node::gen, dt_node::gen_kids, dt_node::gen_kids_1,
dt_operand::gen, dt_operand::gen_gimple_expr,
dt_simplify::gen): Add a param. Rename generated vars.
(decision_tree::insert_operand,
(capture_info::walk_match, capture_info::walk_result,
capture_info::walk_c_expr): Rename local vars.
(expr::gen_transform): Rename generated vars.
Use snprintf. Rename local vars.
(capture::gen_transform, dt_operand::get_name,
dt_operand::gen_opname): Rename generated vars.
(write_predicate): Adjust call to gen_kids.
(parser::get_internal_capture_id): Rename generated vars.
(parser::parse_expr): Rename local vars.
(parser::parse_if): Remove local var.
(parser::parse_pattern, add_operator): Rename local vars.
Index: gcc/genmatch.c
===
--- gcc/genmatch.c (revision 276484)
+++ gcc/genmatch.c (working copy)
@@ -1022,10 +1022,10 @@ commutate (operand *op, vec > _
for (unsigned i = 0; i < result.length (); ++i)
{
expr *ne = new expr (e);
- if (operator_id *p = dyn_cast (ne->operation))
+ if (operator_id *r = dyn_cast (ne->operation))
{
- if (comparison_code_p (p->code))
- ne->operation = swap_tree_comparison (p);
+ if (comparison_code_p (r->code))
+ ne->operation = swap_tree_comparison (r);
}
else if (user_id *p = dyn_cast (ne->operation))
{
@@ -1279,7 +1279,7 @@ lower_cond (operand *o)
|| (is_a (e->ops[0])
&& as_a (e->ops[0])->ops.length () == 2)))
{
- expr *ne = new expr (e);
+ ne = new expr (e);
for (unsigned j = 0; j < result[i].length (); ++j)
ne->append_op (result[i][j]);
if (capture *c = dyn_cast (ne->ops[0]))
@@ -1637,10 +1637,10 @@ class dt_node
unsigned pos);
dt_node *append_simplify (simplify *, unsigned, dt_operand **);
- virtual void gen (FILE *, int, bool) {}
+ virtual void gen (FILE *, int, bool, int) {}
- void gen_kids (FILE *, int, bool);
- void gen_kids_1 (FILE *, int, bool,
+ void gen_kids (FILE *, int, bool, int);
+ void gen_kids_1 (FILE *, int, bool, int,
vec, vec, vec,
vec, vec, vec);
@@ -1663,11 +1663,11 @@ class dt_operand : public dt_node
: dt_node (type, parent_), op (op_), match_dop (match_dop_),
pos (pos_), value_match (false), for_id (current_id) {}
- void gen (FILE *, int, bool);
+ void gen (FILE *, int, bool, int);
unsigned gen_predicate (FILE *, int, const char *, bool);
unsigned gen_match_op (FILE *, int, const char *, bool);
- unsigned gen_gimple_expr (FILE *, int);
+ unsigned gen_gimple_expr (FILE *, int, int);
unsigned gen_generic_expr (FILE *, int, const char *);
char *get_name (char *);
@@ -1689,7 +1689,7 @@ class dt_simplify : public dt_node
indexes (indexes_), info (NULL) {}
void gen_1 (FILE *, int, bool, operand *);
- void gen (FILE *f, int, bool);
+ void gen (FILE *f, int, bool, int);
};
template<>
@@ -1987,9 +1987,9 @@ decision_tree::insert_operand (dt_node *p, operand
if (elm == 0)
{
- dt_operand temp (dt_node::DT_MATCH, 0, match_op, 0, 0);
- temp.value_match = c->value_match;
- elm = decision_tree::find_node (p->kids, );
+ dt_operand match (dt_node::DT_MATCH, 0, match_op, 0, 0);
+ match.value_match = c->value_match;
+ elm = decision_tree::find_node (p->kids, );
}
}
else
@@ -2202,7 +2202,7 @@ capture_info::walk_match (operand *o, unsigned top
for (unsigned i = 0; i < e->ops.length (); ++i)
{
bool cond_p = conditional_p;
- bool cond_expr_cond_p = false;
+ bool expr_cond_p = false;
if (i != 0 && *e->operation == COND_EXPR)
cond_p = true;
else if (*e->operation == TRUTH_ANDIF_EXPR
@@ -2211,8 +2211,8 @@ capture_info::walk_match (operand *o, unsigned top
if (i == 0
&& (*e->operation == COND_EXPR
|| *e->operation == VEC_COND_EXPR))
- cond_expr_cond_p = true;
- walk_match (e->ops[i], toplevel_arg, cond_p, cond_expr_cond_p);
+ expr_cond_p = true;
+ walk_match (e->ops[i], toplevel_arg, cond_p, expr_cond_p);
}
}
