-Wparentheses lumps too much together

-Wparentheses lumps too much together

[jklowden]

Hello Gentlemen, 

Much as I'm a fan of the GCC and rely on -Wall, I would like to suggest 
to you that -Wparentheses should be split up, and things it checks/suggests
be moved out of -Wall.  If this is not the right forum or if you'd rather
see this as a bug report, I'm happy to go where I'm pointed.  

The basic problem with this warning is that it includes some helpful advice 
about some subtle bugs with some "dear beginner" advice about "operators ...
whose precedence people often get confused about."  

My assertion is that operator precedence is fundamental to C.  No one can 
read C without knowing.  If you're unsure about precedence, you're only 
guessing.  Moreover, to interpret operator precedence, it's not necessary
to, say, rememember the variables' declarations 100 lines earlier and 
it's impossible to be misled by non-syntactic indentation.  Everything you
need to know to understand the statement is contained in the statement 
itself.  

My specific candidate for exclusion from -Wall is this one:

	if (a && b || c && d)

which yields (as you know) advice to parenthesize the two && pairs.  

I very much think this is unhelpful, counterproductive advice.
Yes, I know beginners get confused by and/or precedence.  But
*every* language that I know of that has operator precedence places
'and' before 'or'.  More important, a C programmer will encounter
many thousands such expressions in his dealings with the language.
To "help" him is merely to retard his education.

But -Wall isn't really concerned with helping beginners, right?  It's really 
about avoiding errors and pitfalls.  That's why I suggest this and other
precedence warnings be moved to -Woperator (notional).  If you need help 
learning C, GCC will help you.  But if you're maintaining 100,000 lines of 
portable, standards-compliant C (as I do), you don't want warnings to 
parenthesize things that don't need it.  

At the risk of overstaying my welcome, let me answer the commonest reply, 
"what's a few parentheses among friends?"  The problem is that, for the 
experienced programmer, "extra" parenthesis are a signal: something 
unusual is going on here.  By forcing your most careful users --
those who bother with -Wall -- to use parenthesis to enforce what the compiler
would do anyway where the meaning was clear(er) without them, you're 
removing a signalling tool by "insisting" on white noise.  I don't think 
that's a good outcome and I doubt it was the intention, but there it is.  

By the way, I distinguish the 'and/or' advice from the more helpful concern:

	while ((erc=foo()) != 0)

because there the programmer's intent is likely impossible to express 
without parentheses; the idiom requires the override.  

One last point.  In looking for the rationale behind this warning, I searched
for examples of it.  I didn't find any discussion on this list.  What I did
find were many examples of people rototilling perfectly fine code, "improving"
it by adding unneeded parenthesis specifically so that it would compile 
cleanly with -Wall.  I think that's a shame: a waste of effort at best.  

I ask you, please, to consider splitting advice about operator precedence from
advice about mismatched if/else branches, and to exclude advice about 
making sure && is parenthesized ahead of || from -Wall.  -Wall is the 
standard for "good, clean code" in many projects.  This warning doesn't 
belong there.  

Thank you for your time and consideration.  

Regards, 

--jkl

Re: -Wparentheses lumps too much together

[Doug Gregor]

On Dec 19, 2007 3:02 PM,  <jklowden@freetds.org> wrote:
> One last point.  In looking for the rationale behind this warning, I searched
> for examples of it.  I didn't find any discussion on this list.  What I did
> find were many examples of people rototilling perfectly fine code, "improving"
> it by adding unneeded parenthesis specifically so that it would compile
> cleanly with -Wall.  I think that's a shame: a waste of effort at best.
>
> I ask you, please, to consider splitting advice about operator precedence from
> advice about mismatched if/else branches, and to exclude advice about
> making sure && is parenthesized ahead of || from -Wall.  -Wall is the
> standard for "good, clean code" in many projects.  This warning doesn't
> belong there.

For what it is worth, I completely agree with everything you have said
here. This warning oversteps the bounds of what -Wall should do, and
forces people to change perfectly good, clean code. Operator
precedence is an important concept that any C or C++ programmer should
know, and we're not helping anyone by pretending that programmer's
won't understand this concept.

We should certainly remove the warning from -Wall, and perhaps remove
it entirely.

  - Doug

Re: -Wparentheses lumps too much together

[Daniel Jacobowitz]

On Wed, Dec 19, 2007 at 03:02:35PM -0500, jklowden@freetds.org wrote:
> My specific candidate for exclusion from -Wall is this one:
> 
> 	if (a && b || c && d)
> 
> which yields (as you know) advice to parenthesize the two && pairs.  
> 
> I very much think this is unhelpful, counterproductive advice.
> Yes, I know beginners get confused by and/or precedence.  But
> *every* language that I know of that has operator precedence places
> 'and' before 'or'.  More important, a C programmer will encounter
> many thousands such expressions in his dealings with the language.
> To "help" him is merely to retard his education.

I am happy to stand as a counterexample; I am an experienced C
programmer and I greatly appreciate this warning.  And I loathe
reading code which cavalierly assumes you remember the precedence.  +
and *, sure, you learn that in grade school.  && and || is trickier
because there are sensible arguments for both directions; it is
harder to derive from first principles.

If you are more bothered by any clarifying parentheses than I am,
use -Wno-parentheses.

-- 
Daniel Jacobowitz
CodeSourcery

Re: -Wparentheses lumps too much together

[Ismail Dönmez]

Wednesday 19 December 2007 22:11:22 tarihinde Doug Gregor şunları yazmıştı:
> On Dec 19, 2007 3:02 PM,  <jklowden@freetds.org> wrote:
> > One last point.  In looking for the rationale behind this warning, I
> > searched for examples of it.  I didn't find any discussion on this list. 
> > What I did find were many examples of people rototilling perfectly fine
> > code, "improving" it by adding unneeded parenthesis specifically so that
> > it would compile cleanly with -Wall.  I think that's a shame: a waste of
> > effort at best.
> >
> > I ask you, please, to consider splitting advice about operator precedence
> > from advice about mismatched if/else branches, and to exclude advice
> > about making sure && is parenthesized ahead of || from -Wall.  -Wall is
> > the standard for "good, clean code" in many projects.  This warning
> > doesn't belong there.
>
> For what it is worth, I completely agree with everything you have said
> here. This warning oversteps the bounds of what -Wall should do, and
> forces people to change perfectly good, clean code. Operator
> precedence is an important concept that any C or C++ programmer should
> know, and we're not helping anyone by pretending that programmer's
> won't understand this concept.
>
> We should certainly remove the warning from -Wall, and perhaps remove
> it entirely.

Agreed, for example it has lots of useless warnings when compiling ffmpeg.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: -Wparentheses lumps too much together

[Ian Lance Taylor]

jklowden@freetds.org writes:

> Much as I'm a fan of the GCC and rely on -Wall, I would like to suggest 
> to you that -Wparentheses should be split up, and things it checks/suggests
> be moved out of -Wall.  If this is not the right forum or if you'd rather
> see this as a bug report, I'm happy to go where I'm pointed.  

I have no objection to splitting -Wparentheses into separate warnings
controlled by separate options.


> My specific candidate for exclusion from -Wall is this one:
> 
> 	if (a && b || c && d)
> 
> which yields (as you know) advice to parenthesize the two && pairs.  

That particular warning happened to find dozens of real errors when I
ran it over a large code base.  It may be noise for you, but I know
from personal experience that it is very useful.

Ian

Re: -Wparentheses lumps too much together

[James K. Lowden]

Ian Lance Taylor wrote:
> I have no objection to splitting -Wparentheses into separate warnings
> controlled by separate options.

Thank you, Ian.  

> > which yields (as you know) advice to parenthesize the two && pairs.  
> 
> That particular warning happened to find dozens of real errors when I
> ran it over a large code base.  It may be noise for you, but I know
> from personal experience that it is very useful.

I would like to hear more about that, if you wouldn't mind.  I'm really
quite surprised.  Honestly.  

I don't claim to be the last arbiter in good taste.  It sounds like you're
saying that this warning, when applied to code of uncertain quality,
turned up errors -- cases when the code didn't reflect (what must have
been) the programmer's intentions.  My untested (and consequently firmly
held) hypothesis is that 

1) most combinations of && and || don't need parentheses because 

	(a && b) || (c && d) 

is by far more common than

	a && (b || c) && d

and, moreover, broken code fails at runtime, and  

2) Most programmers know (because they need to know) that && comes before
||.  

I'm sure a few years spent working with the GCC and fielding questions
about it would lower my opinion of the average programmer, so I won't try
to convince you.  But I would like to know more about what you found,
because that's at least objective evidence.  I was unable to find any
metrics supporting the inclusion of this particular warning in -Wall.  

I would hold to this, though: the warnings about precedence are of a
different order than warnings about nesting.  I suggest that well vetted
code doesn't benefit from the kind of false positives that -Wparentheses
can generate.  

I very much appreciate your time and effort.  

Kind regards, 

--jkl

Re: -Wparentheses lumps too much together

[Paul Brook]

> My untested (and consequently firmly
> held) hypothesis is that
>
> 1) most combinations of && and || don't need parentheses because
>
> 	(a && b) || (c && d)
>
> is by far more common than
>
> 	a && (b || c) && d
>
> and, moreover, broken code fails at runtime, and

I dispute these claims.

The former may be statistically more common, but I'd be surprised if the 
difference is that big. I can think of several fairly common situations where 
both would be used.

Any time you've got any sort of nontrivial condition, I always find it better 
to include the explicit parentheses. Especially if a, b, c, and d are 
relatively complex relational expressions rather than simple variables.

Code failing at runtime is way too late.  By that time it's already been 
burned onto the device, and may be half way to the moon :-)
Coverage testing never tests everything, and there's a fair chance that your 
complex condition will only break in an exceptional case which is, by 
definition, hard to predict, test and reproduce.

> 2) Most programmers know (because they need to know) that && comes
> before ||. 

I don't really believe that either. Most *good* programmers know operator 
precedence rules (or will at least look it up). However there are a lot of 
distinctly average programmers, and even the good programmers get confused or 
have bad days. As someone else mentioned precedence of arithmetic operators 
is taught in school from a fairly early age. Precedence of logical operators 
is (to me at least) much less well conditioned.


I have no objection to splitting -Wparentheses into finer grained options. I 
just think they should remain enabled by -Wall.

Paul

Re: -Wparentheses lumps too much together

[Ian Lance Taylor]

"James K. Lowden" <jklowden@freetds.org> writes:

> > That particular warning happened to find dozens of real errors when I
> > ran it over a large code base.  It may be noise for you, but I know
> > from personal experience that it is very useful.
> 
> I would like to hear more about that, if you wouldn't mind.  I'm really
> quite surprised.  Honestly.  

I'm not free to share actual details, and I don't have the real
percentages anyhow.

The warning triggered many false positives.  But there were also a
number of true positives, far more than I expected.

A typical true positive looked more or less like

    if (a &&
        b || c)

often with a more complex condition.  The indentation would express
the intent, so it was easy to read the code and assume that it meant
what it appeared to mean.  But, of course, it didn't.

Ian

Re: -Wparentheses lumps too much together

[James K. Lowden]

Ian Lance Taylor wrote:
> A typical true positive looked more or less like
> 
>     if (a &&
>         b || c)

http://www.jetcafe.org/jim/c-style.html

It's funny you should mention that.  A warning about whitespace
indentation that's inconsistent with the expressed logic *would* be
helpful (and consistent with the if/else part of -Wparentheses).  

--jkl

Re: -Wparentheses lumps too much together

[Ralf Wildenhues]

 <jklowden <at> freetds.org> writes:
> 
> Yes, I know beginners get confused by and/or precedence.  But
> *every* language that I know of that has operator precedence places
> 'and' before 'or'.

FWIW, Bourne shell doesn't, && and || have equal precedence there.
That's a bit off-topic though, as it's not an argument against your
actual proposition, but rather one for `sh -Wall'.  ;-)

Cheers,
Ralf

Re: -Wparentheses lumps too much together

[NightStrike]

On 12/20/07, Ralf Wildenhues <Ralf.Wildenhues@gmx.de> wrote:
>  <jklowden <at> freetds.org> writes:
> >
> > Yes, I know beginners get confused by and/or precedence.  But
> > *every* language that I know of that has operator precedence places
> > 'and' before 'or'.
>
> FWIW, Bourne shell doesn't, && and || have equal precedence there.
> That's a bit off-topic though, as it's not an argument against your
> actual proposition, but rather one for `sh -Wall'.  ;-)

It's not entirely off-topic.  Not all programmers are dedicated to a
specific language.  It's customary to work on several different
languages, and keeping things like operator precedance straight in
your head between languages is not always easy.  Things like -Wall are
a great help in making sure that you don't miss any of those
inter-language oddities.

As long as there are options to go either way, for instance:

o  -Wall checks by default, -Wno-parentheses disables
o  -Wall doesn't check by default, -Wparentheses enables

then it's really just a question of what should be enabled by default,
not what should be checked for at all.  The point is... does it really
matter, as long as everyone can go either way?

Re: -Wparentheses lumps too much together

[Ross Smith]

Paul Brook wrote:
 >James K. Lowden wrote:
>>
>> 1) most combinations of && and || don't need parentheses because
>>
>> 	(a && b) || (c && d)
>>
>> is by far more common than
>>
>> 	a && (b || c) && d
>>
>> and, moreover, broken code fails at runtime, and
> 
> I dispute these claims.
> 
> The former may be statistically more common, but I'd be surprised if the 
> difference is that big. I can think of several fairly common situations where 
> both would be used.
> 
> Any time you've got any sort of nontrivial condition, I always find it better 
> to include the explicit parentheses. Especially if a, b, c, and d are 
> relatively complex relational expressions rather than simple variables.

I second Paul's points. The precedence of && and || are not widely 
enough known that warning about it should be off by default (for people 
sane enough to use -Wall).

A couple of data points:

First, I've been writing C and C++ for a living for nearly 20 years now, 
and I didn't know that && had higher precedence than ||. I vaguely 
recalled that they had different precedence, but I couldn't have told 
you which came first without looking it up. I'd happily bet that the 
same is true of the overwhelming majority of developers who aren't 
compiler hackers. Giving && and || different precedence is one of those 
things that feels so totally counterintuitive that I have trouble 
remembering it no matter how many times I look it up. I have a firm 
coding rule of always parenthesising them when they're used together. 
(Likewise &, |, and ^, which have similar issues. I can't remember 
whether -Wparentheses warns about those too.)

Second, I just grepped the codebase I'm curently working on (about 60k 
lines of C++) for anywhere && and || appear on the same line. I got 49 
hits, 29 where && was evaluated before ||, 20 the other way around. (All 
of them, I'm happy to say, properly parenthesised.) So while 
&&-inside-|| seems to be slightly more common, I'd certainly dispute 
James's claim that it's "far more common".

-- Ross Smith

Re: -Wparentheses lumps too much together

[Rehno Lindeque]

> > Yes, I know beginners get confused by and/or precedence.  But
> > *every* language that I know of that has operator precedence places
> > 'and' before 'or'.
>
> FWIW, Bourne shell doesn't, && and || have equal precedence there.
> That's a bit off-topic though, as it's not an argument against your
> actual proposition, but rather one for `sh -Wall'.  ;-)

> It's not entirely off-topic.  Not all programmers are dedicated to a
> specific language.  It's customary to work on several different
> languages, and keeping things like operator precedance straight in
> your head between languages is not always easy.  Things like -Wall are
> a great help in making sure that you don't miss any of those
> inter-language oddities.

Just a note: Operator precedence is taught as logical AND comes before
OR in logic courses. So it is a sort of a standard mathematical
convention just like + and *. In fact, OR is even represented as a +
in some notations. However it might not be practical to assume all
programmers have a background in logic.
-Rehno Lindeque

Re: -Wparentheses lumps too much together

[Joe Buck]

On Fri, Jan 11, 2008 at 09:34:29AM +0200, Rehno Lindeque wrote:
> Just a note: Operator precedence is taught as logical AND comes before
> OR in logic courses. So it is a sort of a standard mathematical
> convention just like + and *. In fact, OR is even represented as a +
> in some notations. However it might not be practical to assume all
> programmers have a background in logic.

A warning that flagged code like

if (c1 || c2 && c3)
   ...

would swamp users in warnings, since this kind of code is extremely
common, and this isn't the kind of thing that anyone who's not a total C
beginner has trouble with.

Re: -Wparentheses lumps too much together

[Robert Dewar]

Joe Buck wrote:
> On Fri, Jan 11, 2008 at 09:34:29AM +0200, Rehno Lindeque wrote:
>> Just a note: Operator precedence is taught as logical AND comes before
>> OR in logic courses. So it is a sort of a standard mathematical
>> convention just like + and *. In fact, OR is even represented as a +
>> in some notations. However it might not be practical to assume all
>> programmers have a background in logic.
> 
> A warning that flagged code like
> 
> if (c1 || c2 && c3)
>    ...
> 
> would swamp users in warnings, since this kind of code is extremely
> common, and this isn't the kind of thing that anyone who's not a total C
> beginner has trouble with.

I find it a valuable warning, I think it is a mistake to rely on
dubious precedence for cases like this (dubious because the analogy
of || and && with + and * is stretching things).

I must say I prefer the Ada syntax here, which prohibits mixing
of different logical operators in this way.

Re: -Wparentheses lumps too much together

[Doug Gregor]

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

On Dec 19, 2007 3:02 PM,  <jklowden@freetds.org> wrote:
> My specific candidate for exclusion from -Wall is this one:
>
>         if (a && b || c && d)
>
> which yields (as you know) advice to parenthesize the two && pairs.

To make this discussion a bit more concrete, the attached patch
removes this particular warning from -Wparentheses and puts it into a
new warning, -Wprecedence, that is not enabled by -Wall. This is
slightly more fine-grained than what -Wparentheses does now. Opinions?

  - Doug

2008-01-11  Douglas Gregor  <doug.gregor@gmail.com>

	* invoke.texi: Document Wprecedence.

2008-01-11  Douglas Gregor  <doug.gregor@gmail.com>

	* gcc.dg/Wparentheses-1.c: Use -Wprecedence
	* gcc.dg/Wparentheses-5.c: Ditto.
	* g++.dg/warn/Wparentheses-8.C: Ditto.
	* g++.dg/warn/Wparentheses-17.C: Ditto.
	* g++.dg/warn/Wparentheses-5.C: Ditto.
	
2008-01-11  Douglas Gregor  <doug.gregor@gmail.com>

	* typeck.c (build_x_binary_op): Call warn_about_parentheses if
	either warn_parentheses or warn_precedence.
	(convert_for_assignment): Ditto.

2008-01-11  Douglas Gregor  <doug.gregor@gmail.com>

	* c.opt (Wprecedence): Add new warning category.
	* c-typeck.c (parser_build_binary_op): Call warn_about_parentheses
	if either warn_parentheses or warn_precedence.
	(c-common.c): Use Wprecedence for the warning about && and ||.

[-- Attachment #2: Wprecedence.patch --]
[-- Type: application/octet-stream, Size: 6897 bytes --]

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 131476)
+++ doc/invoke.texi	(working copy)
@@ -248,7 +248,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-multichar  -Wnonnull  -Wno-overflow @gol
 -Woverlength-strings  -Wpacked  -Wpadded @gol
 -Wparentheses  -Wpointer-arith  -Wno-pointer-to-int-cast @gol
--Wredundant-decls @gol
+-Wprecedence -Wredundant-decls @gol
 -Wreturn-type  -Wsequence-point  -Wshadow @gol
 -Wsign-compare  -Wsign-conversion  -Wstack-protector @gol
 -Wstrict-aliasing -Wstrict-aliasing=n @gol
@@ -2941,6 +2941,20 @@ look like this:
 
 This warning is enabled by @option{-Wall}.
 
+@item -Wprecedence
+@opindex Wprecedence
+Warn if parentheses are omitted in when using @samp{&&} and @samp{||} 
+together, where the precedence of these operators may be confusing.
+
+@smallexample
+@group
+@{
+  if (a && b || c && d) 
+    foo ();
+@}
+@end group
+@end smallexample
+
 @item -Wsequence-point
 @opindex Wsequence-point
 Warn about code that may have undefined semantics because of violations
Index: testsuite/gcc.dg/Wparentheses-1.c
===================================================================
--- testsuite/gcc.dg/Wparentheses-1.c	(revision 131476)
+++ testsuite/gcc.dg/Wparentheses-1.c	(working copy)
@@ -1,7 +1,7 @@
 /* Copyright (C) 2001 Free Software Foundation, Inc.  */
 
 /* { dg-do compile } */
-/* { dg-options -Wparentheses } */
+/* { dg-options "-Wprecedence" } */
 
 /* Source: Neil Booth, 1 Nov 2001.  PR 3170, 3422 - bogus warnings
    about suggesting parentheses.  */
Index: testsuite/gcc.dg/Wparentheses-5.c
===================================================================
--- testsuite/gcc.dg/Wparentheses-5.c	(revision 131476)
+++ testsuite/gcc.dg/Wparentheses-5.c	(working copy)
@@ -1,9 +1,9 @@
-/* Test operation of -Wparentheses.  Precedence warnings.  && inside
+/* Test operation of -Wprecedence.  Precedence warnings.  && inside
    ||.  */
 /* Origin: Joseph Myers <jsm@polyomino.org.uk> */
 
 /* { dg-do compile } */
-/* { dg-options "-Wparentheses" } */
+/* { dg-options -Wprecedence } */
 
 int foo (int);
 
Index: testsuite/g++.dg/warn/Wparentheses-8.C
===================================================================
--- testsuite/g++.dg/warn/Wparentheses-8.C	(revision 131476)
+++ testsuite/g++.dg/warn/Wparentheses-8.C	(working copy)
@@ -1,5 +1,5 @@
 // { dg-do compile }
-// { dg-options "-Wparentheses" }
+// { dg-options -Wprecedence }
 
 // C++ version of gcc.dg/Wparentheses-5.c
 
Index: testsuite/g++.dg/warn/Wparentheses-17.C
===================================================================
--- testsuite/g++.dg/warn/Wparentheses-17.C	(revision 131476)
+++ testsuite/g++.dg/warn/Wparentheses-17.C	(working copy)
@@ -1,5 +1,5 @@
 // { dg-do compile }
-// { dg-options "-Wparentheses" }
+// { dg-options -Wprecedence }
 
 // Template version of Wparentheses-8.C.
 
Index: testsuite/g++.dg/warn/Wparentheses-5.C
===================================================================
--- testsuite/g++.dg/warn/Wparentheses-5.C	(revision 131476)
+++ testsuite/g++.dg/warn/Wparentheses-5.C	(working copy)
@@ -1,5 +1,5 @@
 // { dg-do compile }
-// { dg-options -Wparentheses }
+// { dg-options "-Wparentheses -Wprecedence" }
 
 // C++ version of gcc.dg/Wparentheses-1.c.
 
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 131476)
+++ cp/typeck.c	(working copy)
@@ -2979,7 +2979,7 @@ build_x_binary_op (enum tree_code code, 
   /* Check for cases such as x+y<<z which users are likely to
      misinterpret.  But don't warn about obj << x + y, since that is a
      common idiom for I/O.  */
-  if (warn_parentheses
+  if ((warn_parentheses || warn_precedence)
       && !processing_template_decl
       && !error_operand_p (arg1)
       && !error_operand_p (arg2)
@@ -6430,7 +6430,7 @@ convert_for_assignment (tree type, tree 
 
   /* If -Wparentheses, warn about a = b = c when a has type bool and b
      does not.  */
-  if (warn_parentheses
+  if ((warn_parentheses || warn_precedence)
       && type == boolean_type_node
       && TREE_CODE (rhs) == MODIFY_EXPR
       && !TREE_NO_WARNING (rhs)
Index: c.opt
===================================================================
--- c.opt	(revision 131476)
+++ c.opt	(working copy)
@@ -377,6 +377,10 @@ Wpragmas
 C ObjC C++ ObjC++ Var(warn_pragmas) Init(1) Warning
 Warn about misuses of pragmas
 
+Wprecedence
+C ObjC C++ ObjC++ Var(warn_precedence) Init(0) Warning
+Warn about confusing operator precedence with && and ||
+
 Wprotocol
 ObjC ObjC++ Var(warn_protocol) Init(1) Warning
 Warn if inherited methods are unimplemented
Index: c-typeck.c
===================================================================
--- c-typeck.c	(revision 131476)
+++ c-typeck.c	(working copy)
@@ -2755,7 +2755,7 @@ parser_build_binary_op (enum tree_code c
 
   /* Check for cases such as x+y<<z which users are likely
      to misinterpret.  */
-  if (warn_parentheses)
+  if (warn_parentheses || warn_precedence)
     warn_about_parentheses (code, code1, code2);
 
   if (code1 != tcc_comparison)
Index: c-common.c
===================================================================
--- c-common.c	(revision 131476)
+++ c-common.c	(working copy)
@@ -7249,19 +7249,20 @@ warn_array_subscript_with_type_char (tre
     warning (OPT_Wchar_subscripts, "array subscript has type %<char%>");
 }
 
-/* Implement -Wparentheses for the unexpected C precedence rules, to
-   cover cases like x + y << z which readers are likely to
-   misinterpret.  We have seen an expression in which CODE is a binary
-   operator used to combine expressions headed by CODE_LEFT and
-   CODE_RIGHT.  CODE_LEFT and CODE_RIGHT may be ERROR_MARK, which
-   means that that side of the expression was not formed using a
-   binary operator, or it was enclosed in parentheses.  */
+/* Implement -Wparentheses and -Wprecedence for the unexpected C
+   precedence rules, to cover cases like x + y << z which readers are
+   likely to misinterpret.  We have seen an expression in which CODE
+   is a binary operator used to combine expressions headed by
+   CODE_LEFT and CODE_RIGHT.  CODE_LEFT and CODE_RIGHT may be
+   ERROR_MARK, which means that that side of the expression was not
+   formed using a binary operator, or it was enclosed in
+   parentheses.  */
 
 void
 warn_about_parentheses (enum tree_code code, enum tree_code code_left,
 			enum tree_code code_right)
 {
-  if (!warn_parentheses)
+  if (!warn_parentheses && !warn_precedence)
     return;
 
   if (code == LSHIFT_EXPR || code == RSHIFT_EXPR)
@@ -7276,7 +7277,7 @@ warn_about_parentheses (enum tree_code c
     {
       if (code_left == TRUTH_ANDIF_EXPR
 	  || code_right == TRUTH_ANDIF_EXPR)
-	warning (OPT_Wparentheses,
+	warning (OPT_Wprecedence,
 		 "suggest parentheses around && within ||");
     }
 

Re: -Wparentheses lumps too much together

[Ian Lance Taylor]

Joe Buck <Joe.Buck@synopsys.COM> writes:

> A warning that flagged code like
> 
> if (c1 || c2 && c3)
>    ...
> 
> would swamp users in warnings, since this kind of code is extremely
> common, and this isn't the kind of thing that anyone who's not a total C
> beginner has trouble with.

That is what -Wparentheses does today.

I think it's a good thing, because I've seen it catch coding errors in
real code.

Ian

Re: -Wparentheses lumps too much together

[Joe Buck]

On Fri, Jan 11, 2008 at 03:24:46PM -0800, Ian Lance Taylor wrote:
> Joe Buck <Joe.Buck@synopsys.COM> writes:
> 
> > A warning that flagged code like
> > 
> > if (c1 || c2 && c3)
> >    ...
> > 
> > would swamp users in warnings, since this kind of code is extremely
> > common, and this isn't the kind of thing that anyone who's not a total C
> > beginner has trouble with.
> 
> That is what -Wparentheses does today.
> 
> I think it's a good thing, because I've seen it catch coding errors in
> real code.

It appears that I was mistaken.  Sorry for the noise.

Re: -Wparentheses lumps too much together

[Ian Lance Taylor]

"Doug Gregor" <doug.gregor@gmail.com> writes:

> To make this discussion a bit more concrete, the attached patch
> removes this particular warning from -Wparentheses and puts it into a
> new warning, -Wprecedence, that is not enabled by -Wall. This is
> slightly more fine-grained than what -Wparentheses does now. Opinions?

Personally, I think it should stay in -Wall.  But I'm willing to hear
other opinions.


> @@ -6430,7 +6430,7 @@ convert_for_assignment (tree type, tree 
>  
>    /* If -Wparentheses, warn about a = b = c when a has type bool and b
>       does not.  */
> -  if (warn_parentheses
> +  if ((warn_parentheses || warn_precedence)
>        && type == boolean_type_node
>        && TREE_CODE (rhs) == MODIFY_EXPR
>        && !TREE_NO_WARNING (rhs)

I believe this case (in cp/typeck.c) should only check
warn_parentheses, not warn_precedence.


I'm inclined to approve this if -Wprecedence stays in -Wall, but I'd
like to hear if anybody else has anything to say.

Ian

Re: -Wparentheses lumps too much together

[Andreas Schwab]

Ian Lance Taylor <iant@google.com> writes:

> I'm inclined to approve this if -Wprecedence stays in -Wall, but I'd
> like to hear if anybody else has anything to say.

The name of the option is rather poor, IMHO.  -Wparentheses warns about
precedences, so what is the difference to -Wprecedence?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: -Wparentheses lumps too much together

[Paolo Bonzini]

Andreas Schwab wrote:
> Ian Lance Taylor <iant@google.com> writes:
> 
>> I'm inclined to approve this if -Wprecedence stays in -Wall, but I'd
>> like to hear if anybody else has anything to say.
> 
> The name of the option is rather poor, IMHO.  -Wparentheses warns about
> precedences, so what is the difference to -Wprecedence?

-Wparentheses warns in general about things that the GCS prefer to be 
parenthesized.  I would prefer something like

int warn_precedence = 2;

...

if (warn_precedence == 2)
   warn_precedence = warn_parentheses;

that is, we have

   -Wparentheses remains as is now, and is enabled by -Wall
   -Wparentheses -Wno-precedence only warns about things like a = b = c
                                 for bool b and non-bool a

Paolo

Re: -Wparentheses lumps too much together

[Gabriel Dos Reis]

Ian Lance Taylor <iant@google.com> writes:

| "Doug Gregor" <doug.gregor@gmail.com> writes:
| 
| > To make this discussion a bit more concrete, the attached patch
| > removes this particular warning from -Wparentheses and puts it into a
| > new warning, -Wprecedence, that is not enabled by -Wall. This is
| > slightly more fine-grained than what -Wparentheses does now. Opinions?
| 
| Personally, I think it should stay in -Wall.  But I'm willing to hear
| other opinions.

I agree that the warning should stay in -Wall. However, we may
consider giving that group a more suggestive name, such as
-Wprecedence/-Wno-precedence (enabled by default).

-- Gaby

Re: -Wparentheses lumps too much together

[Derek M Jones]

All,

Developer knowledge of operator precedence and the issue of what
they intended to write are interesting topics.  Some experimental
work is described in (binary operators only I'm afraid):

www.knosof.co.uk/cbook/accu06a.pdf
www.knosof.co.uk/cbook/accu07a.pdf

The ACCU 2006 experiment provides evidence that developer knowledge
is proportional to the number of occurrences of a construct in
source code, it also shows a stunningly high percentage of incorrect
answers.

The ACCU 2007 experiment provides evidence that the names of the
operands has a significant impact on operator precedence choice.

The data from the ACCU06 experiment might be used to select a cutoff
above (ie, frequency of occurrence or developer performance) which
operator pairs will not be flagged as requiring parenthesis.

If GCC wanted to be even more selective it could look at the operand
names before deciding whether to complain.

ps.
I am always on he look out for opportunities to run experiments
using experienced developers.  Does anybody have any suggestions
for conferences I might approach?

-- 
Derek M. Jones                              tel: +44 (0) 1252 520 667
Knowledge Software Ltd                      mailto:derek@knosof.co.uk
Applications Standards Conformance Testing    http://www.knosof.co.uk