Patchwork tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

login
register
mail settings
Submitter JonY
Date April 10, 2012, 12:15 p.m.
Message ID <4F842444.4050309@users.sourceforge.net>
Download mbox | patch
Permalink /patch/151534/
State New
Headers show

Comments

JonY - April 10, 2012, 12:15 p.m.
Hi,

Patch OK?

ChangeLog:
tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings.
Richard Guenther - April 10, 2012, 12:37 p.m.
On Tue, Apr 10, 2012 at 2:15 PM, JonY <jon_y@users.sourceforge.net> wrote:
> Hi,
>
> Patch OK?

What kind of warning?

> ChangeLog:
> tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings.
>
> Index: tree-parloops.c
> ===================================================================
> --- tree-parloops.c     (revision 186243)
> +++ tree-parloops.c     (working copy)
> @@ -723,13 +723,15 @@
>   FOR_EACH_VEC_ELT (basic_block, body, i, bb)
>     if (bb != entry_bb && bb != exit_bb)
>       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -       if (is_gimple_debug (gsi_stmt (gsi)))
> -         {
> -           if (gimple_debug_bind_p (gsi_stmt (gsi)))
> -             has_debug_stmt = true;
> -         }
> -       else
> -         eliminate_local_variables_stmt (entry, &gsi, decl_address);
> +        {
> +         if (is_gimple_debug (gsi_stmt (gsi)))
> +           {
> +             if (gimple_debug_bind_p (gsi_stmt (gsi)))
> +               has_debug_stmt = true;
> +           }
> +         else
> +           eliminate_local_variables_stmt (entry, &gsi, decl_address);
> +        }
>
>   if (has_debug_stmt)
>     FOR_EACH_VEC_ELT (basic_block, body, i, bb)
>
JonY - April 10, 2012, 1:06 p.m.
On 4/10/2012 20:37, Richard Guenther wrote:
> On Tue, Apr 10, 2012 at 2:15 PM, JonY wrote:
>> Hi,
>>
>> Patch OK?
> 
> What kind of warning?
> 

Oops, I forgot to mention gcc was complaining about missing braces.

>> ChangeLog:
>> tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings.
>>
>> Index: tree-parloops.c
>> ===================================================================
>> --- tree-parloops.c     (revision 186243)
>> +++ tree-parloops.c     (working copy)
>> @@ -723,13 +723,15 @@
>>   FOR_EACH_VEC_ELT (basic_block, body, i, bb)
>>     if (bb != entry_bb && bb != exit_bb)
>>       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> -       if (is_gimple_debug (gsi_stmt (gsi)))
>> -         {
>> -           if (gimple_debug_bind_p (gsi_stmt (gsi)))
>> -             has_debug_stmt = true;
>> -         }
>> -       else
>> -         eliminate_local_variables_stmt (entry, &gsi, decl_address);
>> +        {
>> +         if (is_gimple_debug (gsi_stmt (gsi)))
>> +           {
>> +             if (gimple_debug_bind_p (gsi_stmt (gsi)))
>> +               has_debug_stmt = true;
>> +           }
>> +         else
>> +           eliminate_local_variables_stmt (entry, &gsi, decl_address);
>> +        }
>>
>>   if (has_debug_stmt)
>>     FOR_EACH_VEC_ELT (basic_block, body, i, bb)
>>
>
Richard Guenther - April 10, 2012, 2:38 p.m.
On Tue, Apr 10, 2012 at 3:06 PM, JonY <jon_y@users.sourceforge.net> wrote:
> On 4/10/2012 20:37, Richard Guenther wrote:
>> On Tue, Apr 10, 2012 at 2:15 PM, JonY wrote:
>>> Hi,
>>>
>>> Patch OK?
>>
>> What kind of warning?
>>
>
> Oops, I forgot to mention gcc was complaining about missing braces.

Not for me.  And I fail to see why it should.

Richard.

>>> ChangeLog:
>>> tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings.
>>>
>>> Index: tree-parloops.c
>>> ===================================================================
>>> --- tree-parloops.c     (revision 186243)
>>> +++ tree-parloops.c     (working copy)
>>> @@ -723,13 +723,15 @@
>>>   FOR_EACH_VEC_ELT (basic_block, body, i, bb)
>>>     if (bb != entry_bb && bb != exit_bb)
>>>       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>> -       if (is_gimple_debug (gsi_stmt (gsi)))
>>> -         {
>>> -           if (gimple_debug_bind_p (gsi_stmt (gsi)))
>>> -             has_debug_stmt = true;
>>> -         }
>>> -       else
>>> -         eliminate_local_variables_stmt (entry, &gsi, decl_address);
>>> +        {
>>> +         if (is_gimple_debug (gsi_stmt (gsi)))
>>> +           {
>>> +             if (gimple_debug_bind_p (gsi_stmt (gsi)))
>>> +               has_debug_stmt = true;
>>> +           }
>>> +         else
>>> +           eliminate_local_variables_stmt (entry, &gsi, decl_address);
>>> +        }
>>>
>>>   if (has_debug_stmt)
>>>     FOR_EACH_VEC_ELT (basic_block, body, i, bb)
>>>
>>
>
>
NightStrike - April 10, 2012, 7:08 p.m.
On Tue, Apr 10, 2012 at 10:38 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 3:06 PM, JonY <jon_y@users.sourceforge.net> wrote:
>> On 4/10/2012 20:37, Richard Guenther wrote:
>>> On Tue, Apr 10, 2012 at 2:15 PM, JonY wrote:
>>>> Hi,
>>>>
>>>> Patch OK?
>>>
>>> What kind of warning?
>>>
>>
>> Oops, I forgot to mention gcc was complaining about missing braces.
>
> Not for me.  And I fail to see why it should.

This is the warning:

../../../../build/gcc/src/gcc/tree-parloops.c:724: warning: suggest
explicit braces to avoid ambiguous `else'
Richard Guenther - April 11, 2012, 8:46 a.m.
On Tue, Apr 10, 2012 at 9:08 PM, NightStrike <nightstrike@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 10:38 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Apr 10, 2012 at 3:06 PM, JonY <jon_y@users.sourceforge.net> wrote:
>>> On 4/10/2012 20:37, Richard Guenther wrote:
>>>> On Tue, Apr 10, 2012 at 2:15 PM, JonY wrote:
>>>>> Hi,
>>>>>
>>>>> Patch OK?
>>>>
>>>> What kind of warning?
>>>>
>>>
>>> Oops, I forgot to mention gcc was complaining about missing braces.
>>
>> Not for me.  And I fail to see why it should.
>
> This is the warning:
>
> ../../../../build/gcc/src/gcc/tree-parloops.c:724: warning: suggest
> explicit braces to avoid ambiguous `else'

> ./xgcc -B. -c   -g -DIN_GCC   -W -Wall -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Wold-style-definition -Wc++-compat -fno-common  -DHAVE_CONFIG_H -I. -I. -I/space/rguenther/src/svn/trunk/gcc -I/space/rguenther/src/svn/trunk/gcc/. -I/space/rguenther/src/svn/trunk/gcc/../include -I/space/rguenther/src/svn/trunk/gcc/../libcpp/include  -I/space/rguenther/src/svn/trunk/gcc/../libdecnumber -I/space/rguenther/src/svn/trunk/gcc/../libdecnumber/bid -I../libdecnumber    /space/rguenther/src/svn/trunk/gcc/tree-parloops.c -o tree-parloops.o

no warning from trunk.  Which GCC version emits this warning?  Note that
we do not try to keep stage1 warning-free but only later stages (which is
why we use -Werror there).

Richard.
NightStrike - April 13, 2012, 12:30 p.m.
On Wed, Apr 11, 2012 at 4:46 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 9:08 PM, NightStrike <nightstrike@gmail.com> wrote:
>> On Tue, Apr 10, 2012 at 10:38 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Tue, Apr 10, 2012 at 3:06 PM, JonY <jon_y@users.sourceforge.net> wrote:
>>>> On 4/10/2012 20:37, Richard Guenther wrote:
>>>>> On Tue, Apr 10, 2012 at 2:15 PM, JonY wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Patch OK?
>>>>>
>>>>> What kind of warning?
>>>>>
>>>>
>>>> Oops, I forgot to mention gcc was complaining about missing braces.
>>>
>>> Not for me.  And I fail to see why it should.
>>
>> This is the warning:
>>
>> ../../../../build/gcc/src/gcc/tree-parloops.c:724: warning: suggest
>> explicit braces to avoid ambiguous `else'
>
>> ./xgcc -B. -c   -g -DIN_GCC   -W -Wall -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Wold-style-definition -Wc++-compat -fno-common  -DHAVE_CONFIG_H -I. -I. -I/space/rguenther/src/svn/trunk/gcc -I/space/rguenther/src/svn/trunk/gcc/. -I/space/rguenther/src/svn/trunk/gcc/../include -I/space/rguenther/src/svn/trunk/gcc/../libcpp/include  -I/space/rguenther/src/svn/trunk/gcc/../libdecnumber -I/space/rguenther/src/svn/trunk/gcc/../libdecnumber/bid -I../libdecnumber    /space/rguenther/src/svn/trunk/gcc/tree-parloops.c -o tree-parloops.o
>
> no warning from trunk.  Which GCC version emits this warning?  Note that
> we do not try to keep stage1 warning-free but only later stages (which is
> why we use -Werror there).
>
> Richard.

Looks like cygwin gcc 3.4.4
Mike Stump - April 13, 2012, 2:20 p.m.
On Apr 13, 2012, at 5:30 AM, NightStrike wrote:
>> no warning from trunk.  Which GCC version emits this warning?

> Looks like cygwin gcc 3.4.4

3.4.4 is a little old now..  We'd encourage an upgrade to a fine new compiler...  :-)
Bernd Schmidt - April 13, 2012, 2:33 p.m.
On 04/13/2012 04:20 PM, Mike Stump wrote:
> On Apr 13, 2012, at 5:30 AM, NightStrike wrote:
>>> no warning from trunk.  Which GCC version emits this warning?
> 
>> Looks like cygwin gcc 3.4.4
> 
> 3.4.4 is a little old now..  We'd encourage an upgrade to a fine new compiler...  :-)

The thing is, the else really is ambiguous, so it looks like we have a
warning regression.


Bernd
Jakub Jelinek - April 13, 2012, 2:44 p.m.
On Fri, Apr 13, 2012 at 04:33:17PM +0200, Bernd Schmidt wrote:
> On 04/13/2012 04:20 PM, Mike Stump wrote:
> > On Apr 13, 2012, at 5:30 AM, NightStrike wrote:
> >>> no warning from trunk.  Which GCC version emits this warning?
> > 
> >> Looks like cygwin gcc 3.4.4
> > 
> > 3.4.4 is a little old now..  We'd encourage an upgrade to a fine new compiler...  :-)
> 
> The thing is, the else really is ambiguous, so it looks like we have a
> warning regression.

To the compiler?  There is for in between, no need to put extra braces IMHO.
I believe it has been intentionally fixed for 4.0, but my bisect tree only
goes back to r90000.

	Jakub
Bernd Schmidt - April 13, 2012, 2:50 p.m.
On 04/13/2012 04:44 PM, Jakub Jelinek wrote:
> On Fri, Apr 13, 2012 at 04:33:17PM +0200, Bernd Schmidt wrote:
>> On 04/13/2012 04:20 PM, Mike Stump wrote:
>>> On Apr 13, 2012, at 5:30 AM, NightStrike wrote:
>>>>> no warning from trunk.  Which GCC version emits this warning?
>>>
>>>> Looks like cygwin gcc 3.4.4
>>>
>>> 3.4.4 is a little old now..  We'd encourage an upgrade to a fine new compiler...  :-)
>>
>> The thing is, the else really is ambiguous, so it looks like we have a
>> warning regression.
> 
> To the compiler?  There is for in between, no need to put extra braces IMHO.
> I believe it has been intentionally fixed for 4.0, but my bisect tree only
> goes back to r90000.

Well, to the compiler even
if ()
  if ()
    x
  else
    y

has an unambiguous meaning. The problem is that a human might be
confused, for example due to bad indentation. Whether there's a for in
between doesn't matter for this purpose, the following is most likely a bug:

if ()
  for (..)
    if ()
      x
else
  y


Bernd
NightStrike - April 13, 2012, 3:17 p.m.
On Fri, Apr 13, 2012 at 10:20 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Apr 13, 2012, at 5:30 AM, NightStrike wrote:
>>> no warning from trunk.  Which GCC version emits this warning?
>
>> Looks like cygwin gcc 3.4.4
>
> 3.4.4 is a little old now..  We'd encourage an upgrade to a fine new compiler...  :-)

Yeah, not sure why cygwin doesn't make the gcc4 packages the default.
NightStrike - April 13, 2012, 3:19 p.m.
On Fri, Apr 13, 2012 at 10:33 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 04/13/2012 04:20 PM, Mike Stump wrote:
>> On Apr 13, 2012, at 5:30 AM, NightStrike wrote:
>>>> no warning from trunk.  Which GCC version emits this warning?
>>
>>> Looks like cygwin gcc 3.4.4
>>
>> 3.4.4 is a little old now..  We'd encourage an upgrade to a fine new compiler...  :-)
>
> The thing is, the else really is ambiguous, so it looks like we have a
> warning regression.

I'd agree here.  It's why I didn't think to try with a new compiler.
Mike Stump - April 13, 2012, 8:16 p.m.
On Apr 13, 2012, at 7:50 AM, Bernd Schmidt wrote:
> The problem is that a human might be
> confused, for example due to bad indentation. Whether there's a for in
> between doesn't matter for this purpose, the following is most likely a bug:
> 
> if ()
>  for (..)
>    if ()
>      x
> else
>  y

I like the warning only when the indentation of the else does not match the indentation of the matching if.  Most humans I think can follow the meaning correctly, if the code is indented correctly.  The point of the warning would be to catch typos, and an else that isn't in the right column, is most likely wrong.  Though, I'd argue that any else in the wrong column, is most likely wrong...
Magnus Fromreide - April 14, 2012, 2:19 a.m.
On Fri, 2012-04-13 at 16:50 +0200, Bernd Schmidt wrote:
> On 04/13/2012 04:44 PM, Jakub Jelinek wrote:
> > On Fri, Apr 13, 2012 at 04:33:17PM +0200, Bernd Schmidt wrote:
> >> On 04/13/2012 04:20 PM, Mike Stump wrote:
> >>> On Apr 13, 2012, at 5:30 AM, NightStrike wrote:
> >>>>> no warning from trunk.  Which GCC version emits this warning?
> >>>
> >>>> Looks like cygwin gcc 3.4.4
> >>>
> >>> 3.4.4 is a little old now..  We'd encourage an upgrade to a fine new compiler...  :-)
> >>
> >> The thing is, the else really is ambiguous, so it looks like we have a
> >> warning regression.
> > 
> > To the compiler?  There is for in between, no need to put extra braces IMHO.
> > I believe it has been intentionally fixed for 4.0, but my bisect tree only
> > goes back to r90000.
> 
> Well, to the compiler even
> if ()
>   if ()
>     x
>   else
>     y
> 
> has an unambiguous meaning. The problem is that a human might be
> confused, for example due to bad indentation.

For me this warning is almost always a false positive since I am unable
to turn it of selectively.

I am programming in C++.
My main use of

if () ; else y

is to get a temporary scope that doesn't have any braces, e.g. in this
macro

#define DEBUG if (!enable_debugging) ; else debug_object()

where debug_object is an instance of std::ostream.
I then use it like

DEBUG << "fancy message goes here";

Now, writing

if (condition)
  DEBUG << "ooops, condition is true";

is not that far-fetched and obviously triggers the warning but I have a
hard time seeing how that is confusing to a casual reader of the code.


(In order to avoid this warning I am currently writing the above code
as

#define DEBUG switch (!enable_debugging) case false: debug_object()

but that makes the define harder to understand and so the net result of
the warning is to reduce the clarity of the code)

/MF

Patch

Index: tree-parloops.c
===================================================================
--- tree-parloops.c     (revision 186243)
+++ tree-parloops.c     (working copy)
@@ -723,13 +723,15 @@ 
   FOR_EACH_VEC_ELT (basic_block, body, i, bb)
     if (bb != entry_bb && bb != exit_bb)
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-       if (is_gimple_debug (gsi_stmt (gsi)))
-         {
-           if (gimple_debug_bind_p (gsi_stmt (gsi)))
-             has_debug_stmt = true;
-         }
-       else
-         eliminate_local_variables_stmt (entry, &gsi, decl_address);
+        {
+         if (is_gimple_debug (gsi_stmt (gsi)))
+           {
+             if (gimple_debug_bind_p (gsi_stmt (gsi)))
+               has_debug_stmt = true;
+           }
+         else
+           eliminate_local_variables_stmt (entry, &gsi, decl_address);
+        }

   if (has_debug_stmt)
     FOR_EACH_VEC_ELT (basic_block, body, i, bb)