[Bug c++/54194] New: GCC 4.8 gives misleading suggestion about arithmetic in operand of '|'

[Bug c++/54194] New: GCC 4.8 gives misleading suggestion about arithmetic in operand of '|'

[csaba_22 at yahoo dot co.uk]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

             Bug #: 54194
           Summary: GCC 4.8 gives misleading suggestion about arithmetic
                    in operand of '|'
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: csaba_22@yahoo.co.uk


Using gcc version 4.8.0 20120806 (experimental) (GCC),
Target: x86_64-unknown-linux-gnu

to compile the following code:

int main()
{
char in[4]={0}, out[6];
out[1] = in[1] & 0x0F | ((in[3] & 0x3C) << 2);
}

results in a misleading warning (misplaced caret):

$ g++-48 -c -Wall -Werror w.cc
w.cc: In function 'int main()':
w.cc:4:45: error: suggest parentheses around arithmetic in operand of '|'
[-Werror=parentheses]
 out[1] = in[1] & 0x0F | ((in[3] & 0x3C) << 2);
                                             ^

The problematic operand is the left operand of '|', but the caret appears to be
pointing at the right operand.

For example, clang version 3.2 (trunk 161319) gives
$ clang++ -c -Wall -Werror w.cc
w.cc:4:16: error: '&' within '|' [-Werror,-Wbitwise-op-parentheses]
out[1] = in[1] & 0x0F | ((in[3] & 0x3C) << 2);
         ~~~~~~^~~~~~ ~
w.cc:4:16: note: place parentheses around the '&' expression to silence this
warning
out[1] = in[1] & 0x0F | ((in[3] & 0x3C) << 2);
               ^
         (           )

[Bug c++/54194] GCC 4.8 gives misleading suggestion about arithmetic in operand of '|'

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-07
                 CC|                            |manu at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-07 15:34:53 UTC ---
Confirmed.

Manu, is there anything that can be done without huge changes?

[Bug c++/54194] GCC 4.8 gives misleading suggestion about arithmetic in operand of '|'

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

--- Comment #2 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-08-07 19:35:45 UTC ---
Use warning_at and pass down the correct location of the operator if possible,
otherwise input_location?

Of course, it would be nice to have ranges but I have given up on that.

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com

--- Comment #3 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-10-05 15:36:35 UTC ---
Ok, I have the straightforward patch changing the position of the caret to the
'|'. This is certainly suboptimal but totally consistent with the existing
warn_about_parentheses infrastructure which generically mentions the operands
and prints *that* operator. I suppose that for long expressions the change can
make for a good improvement.

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

--- Comment #4 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-10-05 16:26:22 UTC ---
Using warning_at is an improvement, yes.

It still doesn't clarify where the parentheses should go, or why the
parentheses are suggested. This is why clang changed the text of the warning. I
seem to remember this as an example of cryptic diagnostic from GCC in some
Clang presentation. Perhaps the massive takedown by Carruth at GoingNative2012
? Bah, I am not going to watch that again just to check.

I personally, find the "& within |" message quite cryptic. My suggestion would
be something like:

warning: precedence of '&' within '|' may be confusing without parentheses
note: if correct, place parentheses around '&' expression to silence this
warning

The warning could point to '|', the note could point to '&'. Alternatively,
there may be a way to say everything in an even shorter line.

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-10-05 16:32:02 UTC ---
Sure. Since the patch simply using warning_at is trivial and anyway
warn_about_parentheses is currently shared with the C front end, I propose to
just do that for 4.8.0. I'm attaching what I have regtested. I think it would
be a definite improvement. Tell me what you think. If you disagree, no problem,
I can move to something else and completely delay this issue to a later time.

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

--- Comment #6 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-10-05 16:32:57 UTC ---
Created attachment 28365
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28365
Tested.

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

--- Comment #7 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-10-05 16:41:51 UTC ---
(In reply to comment #5)
> Tell me what you think. If you disagree, no problem,
> I can move to something else and completely delay this issue to a later time.

Me? I am certainly not going to discard something good to wait for something
perfect. Just saying what may improve over what clang has. But if it was up to
me, I would give you a pre-approval to change any warning() to use either
input_location or a better location, if that is possible, and let you decide
which location is better.

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-10-05 16:47:19 UTC ---
Eh, eh ;) Good, good, I only wanted to make sure we are on the same page on the
issue. For now I'm sending to the mailing list what I have, then we'll see if
we can improve on it.

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

--- Comment #9 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2012-10-09 10:01:37 UTC ---
Author: paolo
Date: Tue Oct  9 10:01:31 2012
New Revision: 192246

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192246
Log:
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

    PR c++/54194
    * tree.h: Add EXPR_LOC_OR_LOC.

c-family/
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

    PR c++/54194
    * c-common.c (warn_about_parentheses): Add location_t parameter;
    use EXPR_LOC_OR_LOC.
    * c-common.h: Update declaration.

c/
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

    PR c++/54194
    * c-typeck.c (parser_build_binary_op): Update warn_about_parentheses
    call.

/cp
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

    PR c++/54194
    * typeck.c (build_x_binary_op): Update warn_about_parentheses call.
    * parser.c (cp_parser_binary_expression): Use SET_EXPR_LOCATION
    on current.lhs.

/testsuite
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

    PR c++/54194
    * g++.dg/warn/Wparentheses-26.C: Adjust.
    * g++.dg/warn/Wparentheses-27.C: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wparentheses-27.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c-common.h
    trunk/gcc/c/ChangeLog
    trunk/gcc/c/c-typeck.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/parser.c
    trunk/gcc/cp/typeck.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/warn/Wparentheses-26.C
    trunk/gcc/tree.h

[Bug c++/54194] misleading suggestion about arithmetic in operand of '|'

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54194

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.8.0

--- Comment #10 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-10-09 10:02:45 UTC ---
Fixed for 4.8.0.