This is the mail archive of the gcc-bugs@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug c/53001] -Wfloat-conversion should be available to warn about floating point errors


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

--- Comment #12 from Manuel LÃpez-IbÃÃez <manu at gcc dot gnu.org> ---
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).

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]