[Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[JonY]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

Hi,

Patch OK?

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)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 196 bytes --]

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Richard Guenther]

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)
>

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[JonY]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

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)
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 196 bytes --]

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Richard Guenther]

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)
>>>
>>
>
>

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[NightStrike]

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'

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Richard Guenther]

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.

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[NightStrike]

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

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Mike Stump]

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...  :-)

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Bernd Schmidt]

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

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Jakub Jelinek]

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

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Bernd Schmidt]

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

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Mike Stump]

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...

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[Magnus Fromreide]

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

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[NightStrike]

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.

Re: [Patch] tree-parloops.c (eliminate_local_variables): Add braces to suppress warnings

[NightStrike]

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.