Bug 34198 - -Wconversion gives apparent erroneous warning with g++ 4.3-20071109
Summary: -Wconversion gives apparent erroneous warning with g++ 4.3-20071109
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: ---
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-22 21:31 UTC by Tom Browder
Modified: 2007-11-24 12:14 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2007-11-22 23:04:36


Attachments
Output from running: g++-4.3-20071109 -v -save-temps -c test_conversion.cc (783 bytes, text/plain)
2007-11-22 21:35 UTC, Tom Browder
Details
Intermediate file produced by g++-4.3-20071109 (124 bytes, text/plain)
2007-11-22 21:37 UTC, Tom Browder
Details
Output from running: g++-4.3-20071109 -v -save-temps -c -Wconversion test_conversion.cc (834 bytes, text/plain)
2007-11-23 02:03 UTC, Tom Browder
Details
Intermediate file produced by g++-4.3-20071109 (124 bytes, text/plain)
2007-11-23 02:05 UTC, Tom Browder
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Browder 2007-11-22 21:31:06 UTC
Consider the following code:

f.cc ==>
void f(const unsigned char b)
{
 unsigned char c = static_cast<unsigned char>(b & 0xff);
}
<== f.cc

Compile with g++ 4.1.2:

 $ g++-4.3-20071109 -c f.cc -Wconversion
 $

Note no warnings.

Compile with g++ 4.3-20071109:

 $ g++-4.3-20071109 -c f.cc -Wconversion
 f.cc: In function 'void f(unsigned char)':
 f.cc:3: warning: conversion to 'unsigned char' from 'int' may alter its value
Comment 1 Tom Browder 2007-11-22 21:35:32 UTC
Created attachment 14616 [details]
Output from running: g++-4.3-20071109 -v -save-temps -c test_conversion.cc
Comment 2 Tom Browder 2007-11-22 21:37:43 UTC
Created attachment 14617 [details]
Intermediate file produced by g++-4.3-20071109
Comment 3 David Fang 2007-11-22 21:52:08 UTC
I'm having some issues with this as well (same snapshot):

some more tests:

void f(const unsigned char b)
{
 unsigned char c = static_cast<unsigned char>(b & 0xff);
 unsigned char d = (unsigned char)(b & 0xff);
 char e = static_cast<char>(b & 0xff);
 char f = char(b & 0xff);
}

pr34198.cc:4: error: conversion to 'unsigned char' from 'int' may alter its value
pr34198.cc:5: error: conversion to 'unsigned char' from 'int' may alter its value
pr34198.cc:6: error: conversion to 'unsigned char' from 'int' may alter its value
pr34198.cc:7: error: conversion to 'unsigned char' from 'int' may alter its value

I'm trying to understand all the reasonings: http://gcc.gnu.org/wiki/Wcoercion
IMHO, the explicit casts (static_cast or function-style) should suppress the warnings.
Comment 4 Manuel López-Ibáñez 2007-11-22 22:43:19 UTC
(In reply to comment #3)
> IMHO, the explicit casts (static_cast or function-style) should suppress the
> warnings.

That is not the problem. The explicit casts suppress the warnings for the implicit conversion that occurs in the assignment. However, the C/C++ front-ends create an additional implicit conversion that is applied to 'b' and that I don't see how it can be avoided.

See that if you try with:
void f(const unsigned char b)
{
 unsigned char c = b & 0xff;
}

you get two warnings:

test.C:3: warning: conversion to ‘unsigned char’ from ‘int’ may alter its value
test.C:3: warning: conversion to ‘unsigned char’ from ‘int’ may alter its value

Also if you try:

void f(const unsigned char b)
{
  if (b & 0xff) {
    return;
  }
}

you still get a warning!

test.C:3: warning: conversion of ‘(int)b’ to ‘unsigned char’ from ‘int’ may alter its value

This also affects the C front-end since the code is shared and they seem to do exactly the same thing. Perhaps convert_and_check is called in a place where it shouldn't be called. Or perhaps something is building '(int)b' when it should just leave 'b' alone. Any ideas?
Comment 5 Jakub Jelinek 2007-11-22 23:04:35 UTC
See the shorten code in e.g. c-typeck.c's build_binary_op.
I think there are two possible fixes:
1) if shortening (i.e. final_type != result_type) don't convert_and_check
to result_type, but instead convert to result_type, and warnings_for_convert_and_check to final_type
2) in conversion_warning's != REAL_CST != INTEGER_CST
   TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
   && TREE_CODE (type) == INTEGER_TYPE case use get_narrower to see if
   expr is zero or sign extended some smaller type.
   If say type is unsigned char and expr is (int) b where unsigned char b;
   then we shouldn't really warn, the conversions together can't alter
   its value, nor change sign etc.
I think 2) is strongly preferrable.
Will try a patch tomorrow^H^H^H^H^H^H^H^Htoday.
Comment 6 Tom Browder 2007-11-23 02:03:52 UTC
Created attachment 14623 [details]
Output from running: g++-4.3-20071109 -v -save-temps -c -Wconversion test_conversion.cc
Comment 7 Tom Browder 2007-11-23 02:05:39 UTC
Created attachment 14624 [details]
Intermediate file produced by g++-4.3-20071109
Comment 8 Manuel López-Ibáñez 2007-11-23 04:01:53 UTC
(In reply to comment #5)
> I think 2) is strongly preferrable.
> Will try a patch tomorrow^H^H^H^H^H^H^H^Htoday.

I think so as well. Thanks for taking the bug. Let me know if you need help, lose interest or don't have time anymore to work on this. I want to resolve this as soon as possible.
Comment 9 Manuel López-Ibáñez 2007-11-23 04:19:46 UTC
@Jakub 

BTW, perhaps get_unwidened is more appropriate for this, since it takes into account the target type.
Comment 10 Jakub Jelinek 2007-11-23 13:39:54 UTC
Subject: Bug 34198

Author: jakub
Date: Fri Nov 23 13:39:44 2007
New Revision: 130377

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=130377
Log:
	PR c++/34198
	* c-common.c (conversion_warning): For INTEGER_TYPE to
	INTEGER_TYPE conversions call get_narrower on expr to avoid
	spurious warnings from binop shortening or when the implicit
	conversion can't change the value.

	* gcc.dg/Wconversion-5.c: New test.
	* g++.dg/Wconversion3.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wconversion3.C
    trunk/gcc/testsuite/gcc.dg/Wconversion-5.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-common.c
    trunk/gcc/testsuite/ChangeLog

Comment 11 Jakub Jelinek 2007-11-23 13:43:14 UTC
Fixed.
Comment 12 Manuel López-Ibáñez 2007-11-23 13:58:25 UTC
(In reply to comment #11)
> Fixed.
> 

I think this fix is wrong. Warning about user-provided casts goes against the idea of Wconversion. 

See also
  http://gcc.gnu.org/ml/gcc-patches/2007-11/msg01221.html
for a more detailed explanation: 
Comment 13 Manuel López-Ibáñez 2007-11-23 14:12:59 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > Fixed.
> > 
> 
> I think this fix is wrong. Warning about user-provided casts goes against the
> idea of Wconversion. 
> 

Forget it. I didn't read the testcases correctly. Sorry Jakub and thanks for the patch.
Comment 14 David Fang 2007-11-23 19:10:25 UTC
Yes, thank you all for the quick response.  Will test upcoming snapshot.
Comment 15 Tom Browder 2007-11-24 02:36:48 UTC
Tried:

  g++ -c -Wconversion test_conversion.cc

using g++ trunk of svn version 30392 and had no warnings.