This is a request to add a new warning that warns on the subset of -Wconversion warnings that involve floating point numbers. For example, with -Wfloat-conversion this would cause a warning: int main(int argc, char ** argv) { int i = 3.14; return i; } I think this could mostly be done by modifying gcc/c-family/c-common.c unsafe_conversion_p to add the ability to only warn on conversions where REAL_TYPE or REAL_CST are involved. The reason -Wconversion is not sufficient is that using it can cause hundreds of warnings, almost all of which are integer size warnings (many of which are in libraries (such as libMesh)). I would like to be able to enable something like -Wfloat-conversion that would catch errors involving floating point number conversions, while ignoring the integer size conversions.
(In reply to comment #0) > This is a request to add a new warning that warns on the subset of -Wconversion > warnings that involve floating point numbers. For example, with > -Wfloat-conversion this would cause a warning: Should it also warn for non-literals? int foo(double x) { return x; } > I think this could mostly be done by modifying gcc/c-family/c-common.c > unsafe_conversion_p to add the ability to only warn on conversions where > REAL_TYPE or REAL_CST are involved. Yes, I think it should be easy to implement. You will also need to add a new option to gcc/c.opt and enable -Wfloat-conversion with -Wconversion (grep for OPT_Wimplict and how it handles its suboptions). Unfortunately, I don't have time to work on this, and probably nobody else has, so you could try to submit a patch: http://gcc.gnu.org/contribute.html
Yes, it should also warn for non-constants, and also for other floating decreases in accuracy such as: float foo(double x) { return x; } I should have time to create a patch for this before 4.8 goes into stage 3. Do you think it needs a copyright assignment and if so what paperwork would you need from my employer?
(In reply to comment #2) > I should have time to create a patch for this before 4.8 goes into stage 3. Do > you think it needs a copyright assignment and if so what paperwork would you > need from my employer? I am afraid so. See: https://www.gnu.org/prep/maintain/maintain.html#Legal-Matters Write to gcc@gcc.gnu.org asking for the documents. There are several people there dealing with this matter. Also, let us know if you find any problems.
*** Bug 53603 has been marked as a duplicate of this bug. ***
(In reply to comment #3) > (In reply to comment #2) > > I should have time to create a patch for this before 4.8 goes into stage 3. Do > > you think it needs a copyright assignment and if so what paperwork would you > > need from my employer? > > I am afraid so. See: > https://www.gnu.org/prep/maintain/maintain.html#Legal-Matters > > Write to gcc@gcc.gnu.org asking for the documents. There are several people > there dealing with this matter. Also, let us know if you find any problems. I have written to to gcc@gcc.gnu.org, and submitted the Employer disclaimer to my employer, but my employer is reluctant to sign it. I may not be able to create the patch because of that.
(In reply to comment #5) > > I have written to to gcc@gcc.gnu.org, and submitted the Employer disclaimer to > my employer, but my employer is reluctant to sign it. I may not be able to > create the patch because of that. Write to both gcc@gcc.gnu.org and assign@gnu.org to explain your case. They may be able to work out an arrangement. Sadly, this situation is fairly common.
I wrote the patch for 4.8.1 (which is the easy part), now I will see if I can get approval to submit it through the bureaucracies (the hard part).
Created attachment 30870 [details] Patch to add -Wfloat-conversion option This patch add a -Wfloat-conversion option. It adds an enumeration type called conversion_safety, with a SAFE_CONVERSION being 0, and unsafe ones being non-zero. It changes the unsafe_conversion_p to return this enumeration. This allows the floating point conversions to be handled separately. Someone who understands the c.opt file better than I should check that I did it correctly. -Wconversion implies -Wfloat-conversion. -Wall does not, but it might possibly be worth considering.
Created attachment 30871 [details] some floating conversion issues This tests three of the floating conversion reduction in precision issues. P.S. Copyright assignment paperwork from both myself and my employer for GCC has been sent to copyright-clerk
Thank you for adding this - I will certainly find it useful! May I suggest that this warning be implicitly enabled by -Wextra, (even if it's not enabled by -Wall) ?
Created attachment 30873 [details] Patch to add -Wfloat-conversion option This version also is enabled with -Wextra. $ gcc -o float_test float_test.c $ gcc -Wfloat-conversion -o float_test float_test.c float_test.c: In function ‘main’: float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion] int i = 3.14; ^ float_test.c: In function ‘foo’: float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^ float_test.c: In function ‘foo2’: float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^ $ gcc -Wconversion -o float_test float_test.c float_test.c: In function ‘main’: float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion] int i = 3.14; ^ float_test.c: In function ‘foo’: float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^ float_test.c: In function ‘foo2’: float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^ $ gcc -Wextra -o float_test float_test.c float_test.c: In function ‘main’: float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion] int i = 3.14; ^ float_test.c: In function ‘foo’: float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^ float_test.c: In function ‘foo2’: float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^
The patch looks mostly correct to me, but you will need a few extra details before a maintainer (not me!) can accept it. * You need to bootstrap + run the testsuite. You don't really need to add testcases, because there are quite a few already in the testsuite for Wconversion, but perhaps to be sure that your patch is correct, you could check those and change -Wconversion for -Wfloat-conversion where it should trigger. * You need to write a Changelog and submit to gcc-patches@gcc.gnu.org. When submitting CC dodji@redhat.com who is the diagnostics maintainer. * There are some potential issues in the patch that maintainers may complain diff -ur gcc-4.8.1_orig/gcc/c-family/c-common.c gcc-4.8.1/gcc/c-family/c-common.c --- gcc-4.8.1_orig/gcc/c-family/c-common.c 2013-05-14 14:52:27.000000000 -0600 +++ gcc-4.8.1/gcc/c-family/c-common.c 2013-09-02 13:14:01.000000000 -0600 @@ -294,6 +294,8 @@ {NULL, 0, 0}, }; +enum conversion_safety {SAFE_CONVERSION=0,UNSAFE_OTHER,UNSAFE_SIGN,UNSAFE_REAL}; + Perhaps you should add a bit of documentation of what each value means? Also, you don't seem to use UNSAFE_SIGN. Also, some extra whitespace around =, after ',' and {, and before }. -extern bool unsafe_conversion_p (tree, tree, bool); This function is used in the C++ FE, you cannot make it static. Did you bootstrap the compiler with the patch? + if ((conversion_kind = unsafe_conversion_p (type, expr, true))) I don't think this is going to be accepted. You should split the assignment and the test. +Wfloat-conversion +C ObjC C++ ObjC++ Var(warn_float_conversion) LangEnabledBy(C ObjC C++,Wconversion) EnabledBy(Wextra) This looks ok to me, but you could add ObjC++ in the LangEnabledBy part. +This includes conversions from real to integer, and from higher precision +real to lower precision real values. This option is also enabled by +@option{-Wconversion}. and by @option{-Wextra}! You should also add it to the list of things enabled by -Wextra (look where -Wextra is defined).
Another potential issue is that your patch is against 4.8.1, but this is not a regression, so it is very likely that maintainers will ask you to submit a patch with respect to current trunk: http://www.gnu.org/software/gcc/svn.html
Manuel López-Ibáñez, thank you for the patch review. I will make the changes you requested, and rebase it against svn trunk, and run bootstrap and testsuite. It may be a few weeks before I get this all done (depending on available time). UNSAFE_SIGN is unused, but when I was doing the patch, I noticed that unsafe_conversion_p basically does not report sign conversion errors, instead it does the warning directly inside the function. So there definitely is a distinct unsafe conversion type, but I am not sure if I should remove the constant entirely or not. Either way I will try and document the behavior better.
Created attachment 30882 [details] Patch to add -Wfloat-conversion option Adding documentation, formating better, removing assignment from test. Still based on 4.8.1 (which will be fixed later) Still a static function, C++ seems to be bootstraped fine.
Created attachment 30899 [details] Patch to add -Wfloat-conversion option against trunk This version is against gcc trunk (rev 202818). For trunk, I had to unmake unsafe_conversion_p being static (it is now extern again.) This does not bootstrap trunk yet, because gcc has floating conversion issues and with this being enabled by -Wextra and with -Werror, gcc fails to build.
(In reply to Joshua Cogliati from comment #16) > This does not bootstrap trunk yet, because gcc has floating conversion > issues and with this being enabled by -Wextra and with -Werror, gcc fails to > build. Are those real bugs or normal code?
(In reply to Manuel López-Ibáñez from comment #17) > (In reply to Joshua Cogliati from comment #16) > > This does not bootstrap trunk yet, because gcc has floating conversion > > issues and with this being enabled by -Wextra and with -Werror, gcc fails to > > build. > > Are those real bugs or normal code? So far they seem to be normal code. I'll try eliminating the warning on a few, but if there are too many I'll just split the patch into the basic warning and one that occurs on -Wextra.
Created attachment 30913 [details] Patch to add -Wfloat-conversion option against trunk This version is against gcc trunk (rev 202818). It now bootstraps. It adds about ten casts so that the existing float conversions in gcc are now explicit instead of implicit so that gcc can bootstrap even with the new warning.
Created attachment 30937 [details] Patch to add -Wfloat-conversion option against trunk Added one more changed needed to get it to compile (which only occured if I did an rm -Rf in the build directry, the make bootstrap didn't catch the libcpp warning found.) The only thing I have left is to possibly modify the test scripts (to use -Wfloat-conversion instead of -Wconversion for the relevant tests) and recompile against the current trunk.
(In reply to Joshua Cogliati from comment #20) > Created attachment 30937 [details] > Patch to add -Wfloat-conversion option against trunk > > Added one more changed needed to get it to compile (which only occured if I > did an rm -Rf in the build directry, the make bootstrap didn't catch the > libcpp warning found.) > > The only thing I have left is to possibly modify the test scripts (to use > -Wfloat-conversion instead of -Wconversion for the relevant tests) and > recompile against the current trunk. I would humbly suggest that you first recompile against current trunk, then submit to gcc-patches and CC Dodji and Jason Merrill (you can find the emails in MAINTAINERS) asking for comments before spending time and effort on updating the test cases. The maintainers may ask you to change the name, or they might not like that it triggers so often in GCC (so they may ask you to not enable it with -Wextra), or they might simply not like the patch at all. Please mention that it is your first patch to GCC and your copyright assignment is already in order. People are reluctant to review patches unless the papers are in order since there is a high chance that they will never be committed.
Created attachment 30979 [details] Patch to add -Wfloat-conversion option against trunk This version only does the -Wfloat-conversion. It does not do the -Wextra or the changes needed to get -Wextra to bootstrap. It starts to change the test cases. This work is to satisfy the request on the gcc-patch list.
Created attachment 30980 [details] Patch to fixup gcc float conversions in GCC This changes the float conversions in gcc so that gcc bootstraps if float conversions are warned about with -Wextra. Splitting the patch was requested on gcc-patches. (Note to self: patches were created with svn diff --diff-cmd diff -x -up )
Created attachment 30994 [details] Patch to add -Wfloat-conversion option against trunk with testcases This version modifies a few more testcases.
Created attachment 31014 [details] Patch to add -Wfloat-conversion option against trunk with testcases Changes some formatting and comments as requested on mailing list.
Created attachment 31065 [details] Patch to add -Wfloat-conversion option against trunk with new testcase This adds a new testcase instead of using existing testcases.
Created attachment 31097 [details] Patch to add -Wfloat-conversion option against trunk with new testcase and detailed warnings This uses a detailed warnings in the testcase. I am not sure if this is a good idea or not since it might get invalidated if the types change. It bootstraps and the new testcase works on x86_64-unknown-linux-gnu
Author: manu Date: Wed Nov 20 07:15:40 2013 New Revision: 205090 URL: http://gcc.gnu.org/viewcvs?rev=205090&root=gcc&view=rev Log: 2013-11-19 Joshua J Cogliati <jrincayc@yahoo.com> PR c/53001 Splitting out a -Wfloat-conversion from -Wconversion for conversions that lower floating point number precision or conversion from floating point numbers to integers. gcc/c-family/ * c-common.c (unsafe_conversion_p): Make this function return an enumeration with more detail. (conversion_warning): Use the new return type of unsafe_conversion_p to separately warn either about conversions that lower floating point number precision or about the other kinds of conversions. * c-common.h (enum conversion_safety): New enumeration. (unsafe_conversion_p): switching return type to conversion_safety enumeration. * c.opt: Adding new warning -Wfloat-conversion and enabling it with -Wconversion. gcc/ * doc/invoke.texi: Adding documentation about -Wfloat-conversion. gcc/testsuite/ * c-c++-common/Wfloat-conversion.c: Copies relevant tests from c-c++-common/Wconversion-real.c, gcc.dg/Wconversion-real-integer.c and gcc.dg/pr35635.c into new testcase for conversions that are warned about by -Wfloat-conversion. Added: trunk/gcc/testsuite/c-c++-common/Wfloat-conversion.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-family/c.opt trunk/gcc/doc/invoke.texi trunk/gcc/testsuite/ChangeLog
I guess this is fixed for GCC 4.9.
Yes, it is fixed so far as I am concerned. I checked out gcc trunk 205109, and bootstraped it and tried it out on: int main(int argc, char ** argv) { int i = 3.14; return i; } int foo(double x) { return x; } float foo2(double x) { return x; } and without the flag it didn't warn, but with the flag it did: gcc -Wall -Wfloat-conversion -o float_test float_test.c float_test.c: In function ‘main’: float_test.c:2:3: warning: conversion to ‘int’ alters ‘double’ constant value [-Wfloat-conversion] int i = 3.14; ^ float_test.c: In function ‘foo’: float_test.c:8:3: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^ float_test.c: In function ‘foo2’: float_test.c:12:3: warning: conversion to ‘float’ from ‘double’ may alter its value [-Wfloat-conversion] return x; ^ I think there are two things remaining to be done. 1. http://gcc.gnu.org/gcc-4.9/changes.html does not yet list it as a change, 2. for gcc 4.10 (or what ever the next gcc version is) I think it might be worth considering as a -Wextra or -Wall. That however would be a separate bug number and would require something like the Patch to fixup gcc float conversions in GCC and another patch to turn it on with the flag.