diff mbox

RFA: PATCH: -Wimplicit-double

Message ID 4C812DE8.1010602@codesourcery.com
State New
Headers show

Commit Message

Mark Mitchell Sept. 3, 2010, 5:18 p.m. UTC 
On 9/3/2010 8:59 AM, Mark Mitchell wrote:

> This patch implements -Wimplicit-double, a new warning.  The goal of
> the warning is to warn users when a "float" is implicitly converted to
> a "double". 

Joseph asked me off-list to make a couple of low-level corrections:

(a) Use warning_at where possible
(b) Name the parameter to do_warn_implicit_double "gmsgid" for i18n

Thanks,

Comments

Manuel López-Ibáñez Sept. 3, 2010, 5:43 p.m. UTC | #1 
On 3 September 2010 19:18, Mark Mitchell <mark@codesourcery.com> wrote:
> On 9/3/2010 8:59 AM, Mark Mitchell wrote:
>
>> This patch implements -Wimplicit-double, a new warning.  The goal of
>> the warning is to warn users when a "float" is implicitly converted to
>> a "double".
>
> Joseph asked me off-list to make a couple of low-level corrections:
>
> (a) Use warning_at where possible
> (b) Name the parameter to do_warn_implicit_double "gmsgid" for i18n
>
> Thanks,

This is warned already by Wconversion for both C and C++ and Fortran!
If it doesn't catch all cases, then that should be fixed appropriately
to use existing code as much as possible. And there are testcases
already for this in the testsuite.

The name of the option suggests that existing Wimplicit-int should do
something similar for integers, which is not the case at all. So bad
name. I suggest Wconversion-double or Wdouble-conversion and make this
option enabled by Wconversion  similar to existing option
Wnull-conversion.

So I think this patch is not OK at all.

Cheers,

Manuel.
Manuel López-Ibáñez Sept. 3, 2010, 5:45 p.m. UTC | #2 
On 3 September 2010 19:43, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 3 September 2010 19:18, Mark Mitchell <mark@codesourcery.com> wrote:
>> On 9/3/2010 8:59 AM, Mark Mitchell wrote:
>>
>>> This patch implements -Wimplicit-double, a new warning.  The goal of
>>> the warning is to warn users when a "float" is implicitly converted to
>>> a "double".
>>
>> Joseph asked me off-list to make a couple of low-level corrections:
>>
>> (a) Use warning_at where possible
>> (b) Name the parameter to do_warn_implicit_double "gmsgid" for i18n
>>
>> Thanks,
>
> This is warned already by Wconversion for both C and C++ and Fortran!
> If it doesn't catch all cases, then that should be fixed appropriately
> to use existing code as much as possible. And there are testcases
> already for this in the testsuite.

In fact, the case without prototype is different and is warned by
Wtraditional-conversion. There are also testcases in the testsuite. So
that part seems to be also duplicated.

Cheers,

Manuel.
Mark Mitchell Sept. 3, 2010, 5:51 p.m. UTC | #3 
On 9/3/2010 10:43 AM, Manuel López-Ibáñez wrote:

>>> This patch implements -Wimplicit-double, a new warning.  The goal of
>>> the warning is to warn users when a "float" is implicitly converted to
>>> a "double".

> This is warned already by Wconversion for both C and C++ and Fortran!

The fact that there's another warning option which warns about the same
code is irrelevant.  -Wconversion and -Wtraditional-conversion are
warning about something entirely different, from the user's perspective.

For example, -Wconversion warns about conversions that alter a value.
That's not the point here; this is about a value being altered, it's
about using a type that your hardware doesn't support natively.  It's
entirely possible that a user doesn't want all the warnings that would
come from -Wconversion or -Wtraditional-conversion, but does want this
class of warnings.
Manuel López-Ibáñez Sept. 3, 2010, 6:36 p.m. UTC | #4 
On 3 September 2010 19:51, Mark Mitchell <mark@codesourcery.com> wrote:
> For example, -Wconversion warns about conversions that alter a value.
> That's not the point here; this is about a value being altered, it's
> about using a type that your hardware doesn't support natively.  It's

Fine. I didn't realise it was a conversion from float->double. I see
the point of the warning now. Still, Wimplicit-double is a bad name
given the existing Wimplicit and Wimplicit-int. It could be
Wpromoted-double or Wpromoted-to-double or something that cannot be
confused with Wconversion or Wimplicit.

Cheers,

Manuel.
Mark Mitchell Sept. 3, 2010, 6:41 p.m. UTC | #5 
On 9/3/2010 11:36 AM, Manuel López-Ibáñez wrote:
> On 3 September 2010 19:51, Mark Mitchell <mark@codesourcery.com> wrote:
>> For example, -Wconversion warns about conversions that alter a value.
>> That's not the point here; this is about a value being altered, it's
>> about using a type that your hardware doesn't support natively.  It's
> 
> Fine. I didn't realise it was a conversion from float->double. I see
> the point of the warning now. Still, Wimplicit-double is a bad name

I don't have a strong feeling about the name.  -Wpromoted-double sounds
like promotion *from* double, and -Wpromoted-to-double is pretty
ungainly.  But, that's a bikeshed.

If the patch as a whole is approved, I'll accept whatever name someone
wants to put on it.
Daniel Jacobowitz Sept. 4, 2010, 1:44 a.m. UTC | #6 
On Fri, Sep 03, 2010 at 11:41:24AM -0700, Mark Mitchell wrote:
> On 9/3/2010 11:36 AM, Manuel López-Ibáñez wrote:
> > On 3 September 2010 19:51, Mark Mitchell <mark@codesourcery.com> wrote:
> >> For example, -Wconversion warns about conversions that alter a value.
> >> That's not the point here; this is about a value being altered, it's
> >> about using a type that your hardware doesn't support natively.  It's
> > 
> > Fine. I didn't realise it was a conversion from float->double. I see
> > the point of the warning now. Still, Wimplicit-double is a bad name
> 
> I don't have a strong feeling about the name.  -Wpromoted-double sounds
> like promotion *from* double, and -Wpromoted-to-double is pretty
> ungainly.  But, that's a bikeshed.
> 
> If the patch as a whole is approved, I'll accept whatever name someone
> wants to put on it.

It's not much better, but that argument suggests -Wdouble-promotion.
Gabriel Dos Reis Sept. 4, 2010, 11:54 a.m. UTC | #7 
On Fri, Sep 3, 2010 at 8:44 PM, Daniel Jacobowitz <dan@codesourcery.com> wrote:

>
> It's not much better, but that argument suggests -Wdouble-promotion.

I agree that -Wdouble-promotion is better than -Wimplicit-double.
Patch OK with me.
diff mbox

Patch

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 163782)
+++ gcc/doc/invoke.texi	(working copy)
@@ -241,8 +241,8 @@  Objective-C and Objective-C++ Dialects}.
 -Wno-format-contains-nul -Wno-format-extra-args -Wformat-nonliteral @gol
 -Wformat-security  -Wformat-y2k @gol
 -Wframe-larger-than=@var{len} -Wjump-misses-init -Wignored-qualifiers @gol
--Wimplicit  -Wimplicit-function-declaration  -Wimplicit-int @gol
--Winit-self  -Winline @gol
+-Wimplicit  -Wimplicit-double -Wimplicit-function-declaration @gol
+-Wimplicit-int -Winit-self  -Winline @gol
 -Wno-int-to-pointer-cast -Wno-invalid-offsetof @gol
 -Winvalid-pch -Wlarger-than=@var{len}  -Wunsafe-loop-optimizations @gol
 -Wlogical-op -Wlong-long @gol
@@ -3198,6 +3198,30 @@  enabled by default and it is made into a
 Same as @option{-Wimplicit-int} and @option{-Wimplicit-function-declaration}.
 This warning is enabled by @option{-Wall}.
 
+@item -Wimplicit-double @r{(C and Objective-C only)}
+@opindex Wimplicit-double
+@opindex Wno-implicit-double
+Give a warning when a value of type @code{float} is implicitly
+promoted to @code{double}.  CPUs with a 32-bit ``single-precision''
+floating-point unit implement @code{float} in hardware, but emulate
+@code{double} in software.  On such a machine, doing computations
+using @code{double} values is much more expensive because of the
+overhead required for software emulation.  
+
+It is easy to accidentally do computations with @code{double} because
+floating-point literals are implicitly of type @code{double}.  For
+example, in:
+@smallexample
+@group
+float area(float radius)
+@{
+   return 3.14159 * radius * radius;        
+@}
+@end group
+@end smallexample
+the compiler will perform the entire computation with @code{double}
+because the floating-point literal is a @code{double}.
+
 @item -Wignored-qualifiers @r{(C and C++ only)}
 @opindex Wignored-qualifiers
 @opindex Wno-ignored-qualifiers
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 163782)
+++ gcc/c-family/c.opt	(working copy)
@@ -266,6 +266,10 @@  Wimplicit
 C ObjC Var(warn_implicit) Init(-1) Warning
 Warn about implicit declarations
 
+Wimplicit-double
+C ObjC C++ ObjC++ Var(warn_implicit_double) Warning
+Warn about implicit conversions from \"float\" to \"double\"
+
 Wimplicit-function-declaration
 C ObjC Var(warn_implicit_function_declaration) Init(-1) Warning
 Warn about implicit function declarations
Index: gcc/c-family/ChangeLog
===================================================================
--- gcc/c-family/ChangeLog	(revision 163782)
+++ gcc/c-family/ChangeLog	(working copy)
@@ -1,3 +1,7 @@ 
+2010-09-03  Mark Mitchell  <mark@codesourcery.com>
+
+	* c.opt (Wimplicit-double): New.
+
 2010-09-02  Joseph Myers  <joseph@codesourcery.com>
 
 	* c.opt (Wimport, fall-virtual, falt-external-templates,
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 163782)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,12 @@ 
+2010-09-03  Mark Mitchell  <mark@codesourcery.com>
+
+	* doc/invoke.texi: Document it.
+	* c-typeck.c (convert_arguments): Check for implicit conversions
+	from float to double.
+	(do_warn_implicit_double): New function.
+	(build_conditional_expr): Use it.
+	(build_binary_op): Likewise.
+
 2010-09-02  Ryan Mansfield  <rmansfield@qnx.com>
 
 	* arm.c (arm_override_options): Correct fall-back code to use
Index: gcc/testsuite/gcc.dg/Wimplicit-double.c
===================================================================
--- gcc/testsuite/gcc.dg/Wimplicit-double.c	(revision 0)
+++ gcc/testsuite/gcc.dg/Wimplicit-double.c	(revision 0)
@@ -0,0 +1,104 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wimplicit-double" } */
+
+#include <stddef.h>
+
+/* Some targets do not provide <complex.h> so we define I ourselves.  */
+#define I 1.0iF
+#define ID ((_Complex double)I)
+
+float f;
+double d;
+int i;
+long double ld;
+_Complex float cf;
+_Complex double cd;
+_Complex long double cld;
+size_t s;
+
+extern void unprototyped_fn ();
+extern void varargs_fn (int, ...);
+extern void double_fn (double);
+extern float float_fn (void);
+
+void 
+usual_arithmetic_conversions(void) 
+{
+  float local_f;
+  _Complex float local_cf;
+
+  /* Values of type "float" are implicitly converted to "double" or
+     "long double" due to use in arithmetic with "double" or "long
+     double" operands.  */
+  local_f = f + 1.0;         /* { dg-warning "implicit" } */
+  local_f = f - d;           /* { dg-warning "implicit" } */
+  local_f = 1.0f * 1.0;      /* { dg-warning "implicit" } */
+  local_f = 1.0f / d;        /* { dg-warning "implicit" } */
+
+  local_cf = cf + 1.0;       /* { dg-warning "implicit" } */
+  local_cf = cf - d;         /* { dg-warning "implicit" } */
+  local_cf = cf + 1.0 * ID;  /* { dg-warning "implicit" } */
+  local_cf = cf - cd;        /* { dg-warning "implicit" } */
+  
+  local_f = i ? f : d;       /* { dg-warning "implicit" } */
+  i = f == d;                /* { dg-warning "implicit" } */
+  i = d != f;                /* { dg-warning "implicit" } */
+}
+
+void 
+default_argument_promotion (void) 
+{
+  /* Because there is no prototype, "f" is promoted to "double".  */
+  unprototyped_fn (f); /* { dg-warning "implicit" } */
+  undeclared_fn (f);   /* { dg-warning "implicit" } */
+  /* Because "f" is part of the variable argument list, it is promoted
+     to "double".  */
+  varargs_fn (1, f);   /* { dg-warning "implicit" } */
+}
+
+/* There is no warning when an explicit cast is used to perform the
+   conversion.  */
+
+void
+casts (void) 
+{
+  float local_f;
+  _Complex float local_cf;
+
+  local_f = (double)f + 1.0;                 /* { dg-bogus "implicit" } */
+  local_f = (double)f - d;                   /* { dg-bogus "implicit" } */
+  local_f = (double)1.0f + 1.0;              /* { dg-bogus "implicit" } */
+  local_f = (double)1.0f - d;                /* { dg-bogus "implicit" } */
+
+  local_cf = (_Complex double)cf + 1.0;      /* { dg-bogus "implicit" } */
+  local_cf = (_Complex double)cf - d;        /* { dg-bogus "implicit" } */
+  local_cf = (_Complex double)cf + 1.0 * ID; /* { dg-bogus "implicit" } */
+  local_cf = (_Complex double)cf - cd;       /* { dg-bogus "implicit" } */
+
+  local_f = i ? (double)f : d;               /* { dg-bogus "implicit" } */
+  i = (double)f == d;                        /* { dg-bogus "implicit" } */
+  i = d != (double)f;                        /* { dg-bogus "implicit" } */
+}
+
+/* There is no warning on conversions that occur in assignment (and
+   assignment-like) contexts.  */
+
+void 
+assignments (void)
+{
+  d = f;           /* { dg-bogus "implicit" } */
+  double_fn (f);   /* { dg-bogus "implicit" } */
+  d = float_fn (); /* { dg-bogus "implicit" } */
+}
+
+/* There is no warning in non-evaluated contexts.  */
+
+void
+non_evaluated (void)
+{
+  s = sizeof (f + 1.0);             /* { dg-bogus "implicit" } */
+  s = __alignof__ (f + 1.0);        /* { dg-bogus "implicit" } */
+  d = (__typeof__(f + 1.0))f;       /* { dg-bogus "implicit" } */
+  s = sizeof (i ? f : d);           /* { dg-bogus "implicit" } */
+  s = sizeof (unprototyped_fn (f)); /* { dg-bogus "implicit" } */
+}
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 163782)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,3 +1,7 @@ 
+2010-09-03  Mark Mitchell  <mark@codesourcery.com>
+
+	* gcc.dg/Wimplicit-double.c: New.
+
 2010-09-02  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* g++.dg/debug/dwarf2/nested-2.C: Allow for ! as comment delimiter.
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 163782)
+++ gcc/c-typeck.c	(working copy)
@@ -3106,8 +3106,15 @@  convert_arguments (tree typelist, VEC(tr
 	  if (type_generic)
 	    parmval = val;
 	  else
-	    /* Convert `float' to `double'.  */
-	    parmval = convert (double_type_node, val);
+	    {
+	      /* Convert `float' to `double'.  */
+	      if (warn_implicit_double && !c_inhibit_evaluation_warnings)
+		warning (OPT_Wimplicit_double,
+			 "implicit conversion from %qT to %qT when passing "
+			 "argument to function",
+			 valtype, double_type_node);
+	      parmval = convert (double_type_node, val);
+	    }
 	}
       else if (excess_precision && !type_generic)
 	/* A "double" argument with excess precision being passed
@@ -4029,6 +4036,40 @@  ep_convert_and_check (tree type, tree ex
   return convert (type, expr);
 }
 
+/* RESULT_TYPE is the result of converting TYPE1 and TYPE2 to a common
+   type via c_common_type.  If -Wimplicit-double is in use, and the
+   conditions for warning have been met, issue a warning.  GMSGID is
+   the warning message.  It must have two %T specifiers for the type
+   that was converted (generally "float") and the type to which it was
+   converted (generally "double), respectively.  LOC is the location
+   to which the awrning should refer.  */
+
+static void
+do_warn_implicit_double (tree result_type, tree type1, tree type2,
+			 const char *gmsgid, location_t loc)
+{
+  tree source_type;
+
+  if (!warn_implicit_double)
+    return;
+  /* If the conversion will not occur at run-time, there is no need to
+     warn about it.  */
+  if (c_inhibit_evaluation_warnings)
+    return;
+  if (TYPE_MAIN_VARIANT (result_type) != double_type_node
+      && TYPE_MAIN_VARIANT (result_type) != complex_double_type_node)
+    return;
+  if (TYPE_MAIN_VARIANT (type1) == float_type_node
+      || TYPE_MAIN_VARIANT (type1) == complex_float_type_node)
+    source_type = type1;
+  else if (TYPE_MAIN_VARIANT (type2) == float_type_node
+	   || TYPE_MAIN_VARIANT (type2) == complex_float_type_node)
+    source_type = type2;
+  else
+    return;
+  warning_at (loc, OPT_Wimplicit_double, gmsgid, source_type, result_type);
+}
+
 /* Build and return a conditional expression IFEXP ? OP1 : OP2.  If
    IFEXP_BCP then the condition is a call to __builtin_constant_p, and
    if folded to an integer constant then the unselected half may
@@ -4140,6 +4181,10 @@  build_conditional_expr (location_t colon
 	       || code2 == COMPLEX_TYPE))
     {
       result_type = c_common_type (type1, type2);
+      do_warn_implicit_double (result_type, type1, type2,
+			       "implicit conversion from %qT to %qT to "
+			       "match other result of conditional",
+			       colon_loc);
 
       /* If -Wsign-compare, warn here if type1 and type2 have
 	 different signedness.  We'll promote the signed to unsigned
@@ -9822,6 +9867,11 @@  build_binary_op (location_t location, en
       if (shorten || common || short_compare)
 	{
 	  result_type = c_common_type (type0, type1);
+	  do_warn_implicit_double (result_type, type0, type1,
+				   "implicit conversion from %qT to %qT "
+				   "to match other operand of binary "
+				   "expression",
+				   location);
 	  if (result_type == error_mark_node)
 	    return error_mark_node;
 	}