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

[Segher Boessenkool]

On Mon, Oct 07, 2019 at 05:11:27PM +, Bernd Edlinger wrote:
> On 10/7/19 9:20 AM, Eric Botcazou wrote:
> > No, please, the cure would be much worse than the disease.
> 
> Ack.
> 
> I think the least worse thing would be a pragma in the macro where the 
> shadowing
> variable is declared...

The best thing would be to write this in a way that nothing *can*
accidentally shadow something.


Segher

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

[Bernd Edlinger]

On 10/7/19 9:20 AM, Eric Botcazou wrote:
>> If this ends up acked then please add this to ansidecl.h or
>> somewhere else global as template:
>>
>> template 
>> struct push {
>>   push (T &);
>>   ~push ();
>>   T *m_loc;
>>   T m_val;
>> };
>>
>> because it would be a general solution for _all_ shadow=local warnings?!
> 
> No, please, the cure would be much worse than the disease.
> 

Ack.

I think the least worse thing would be a pragma in the macro where the shadowing
variable is declared...


Bernd.

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

[Eric Botcazou]

> If this ends up acked then please add this to ansidecl.h or
> somewhere else global as template:
> 
> template 
> struct push {
>   push (T &);
>   ~push ();
>   T *m_loc;
>   T m_val;
> };
> 
> because it would be a general solution for _all_ shadow=local warnings?!

No, please, the cure would be much worse than the disease.

-- 
Eric Botcazou

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

[Richard Biener]

On Thu, Oct 3, 2019 at 5:18 PM Bernd Edlinger  wrote:
>
> Hi,
>
> this fixes -Wshadow=local warnings in passes.c.
> The non-trivial part is due to the PUSH_INSERT_PASSES_WITHIN
> is used recursively, and shadows the local value p
> in each invocation.
>
> Fixed by using a helper class that restores the saved content
> of p at the end of the block.
>
> The shadowing variable in ipa_write_summaries can simply be
> removed sine the outer variable of the same name has the
> same type and is not live here, this is a trivial change.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

The class seems to be a poor-mans way to avoid the warning
while the current use is more clear.  You do

 {
   int i;
   {
 // int i; ... oops, warning
 int prev_i_1 = i;
 i = ...

 // fixup
 i = prev_i_1;
   }

Using of a C++ class doesn't make this less obviously worse.
If this ends up acked then please add this to ansidecl.h or
somewhere else global as template:

template 
struct push {
  push (T &);
  ~push ();
  T *m_loc;
  T m_val;
};

because it would be a general solution for _all_ shadow=local warnings?!

Richard.

>
> Thanks
> Bernd.
>

[PATCH] Fix -Wshadow=local warnings in passes.c

[Bernd Edlinger]

Hi,

this fixes -Wshadow=local warnings in passes.c.
The non-trivial part is due to the PUSH_INSERT_PASSES_WITHIN
is used recursively, and shadows the local value p
in each invocation.

Fixed by using a helper class that restores the saved content
of p at the end of the block.

The shadowing variable in ipa_write_summaries can simply be
removed sine the outer variable of the same name has the
same type and is not live here, this is a trivial change.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

2019-10-03  Bernd Edlinger  

	* passes.c (push_pass_state): New helper class.
	(PUSH_INSERT_PASSES_WITHIN): Use push_pass_state.
	(ipa_write_summaries): Remove shadowing local var.

Index: gcc/passes.c
===
--- gcc/passes.c	(revision 276484)
+++ gcc/passes.c	(working copy)
@@ -1491,6 +1491,30 @@ pass_manager::register_pass (struct register_pass_
 }
 }
 
+/* Helper class that is used in PUSH_INSERT_PASSES_WITHIN
+   to push the current state of the local variable "p",
+   and restore it again to the previous state after the
+   current block is closed.  */
+
+class push_pass_state
+{
+  public:
+push_pass_state (opt_pass **)
+: m_val (val)
+{ 
+  m_org = m_val;
+}
+
+~push_pass_state()
+{
+  m_val = m_org;
+}
+
+  private:
+opt_pass **_val;
+opt_pass **m_org;
+};
+
 /* Construct the pass tree.  The sequencing of passes is driven by
the cgraph routines:
 
@@ -1556,7 +1580,8 @@ pass_manager::pass_manager (context *ctxt)
 
 #define PUSH_INSERT_PASSES_WITHIN(PASS) \
   { \
-opt_pass **p = &(PASS ## _1)->sub;
+push_pass_state scope_ ## PASS (p);		\
+p = &(PASS ## _1)->sub;
 
 #define POP_INSERT_PASSES() \
   }
@@ -2703,7 +2728,7 @@ ipa_write_summaries (void)
 
   for (i = order_pos - 1; i >= 0; i--)
 {
-  struct cgraph_node *node = order[i];
+  node = order[i];
 
   if (gimple_has_body_p (node->decl))
 	{