| Message ID | 4F842444.4050309@users.sourceforge.net |
|---|---|
| State | New |
| Headers | show |
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) >
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) >> >
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) >>> >> > >
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'
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.
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
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... :-)
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
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
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
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.
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.
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...
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
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)