[fortran, patch] reduce noise of -Wconversion

[fortran, patch] reduce noise of -Wconversion

[Daniel Franke]

[-- Attachment #1: Type: Text/Plain, Size: 598 bytes --]


Hi all.

attached patch attempts to reduce amount of noise generated by -Wconversion.


gcc/fortran/:
2010-05-08  Daniel Franke  <franke.daniel@gmail.com>

	PR fortran/27866
	PR fortran/35003
	PR fortran/42809
	* intrinc.c (gfc_convert_type_warn): Be more dicriminative
	about conversion warnings.

gcc/testsuite/:
2010-05-08  Daniel Franke  <franke.daniel@gmail.com>

	PR fortran/27866
	PR fortran/35003
	PR fortran/42809
	* gfortran.dg/array_constructor_type_17.f03: Updated match string.
	* gfortran.dg/warn_conversion.f90: New.


Regression tested on i686-pc-linux-gnu.
Ok for trunk?

	Daniel

[-- Attachment #2: warn_conversion.diff --]
[-- Type: text/x-patch, Size: 2825 bytes --]

Index: fortran/intrinsic.c
===================================================================
--- fortran/intrinsic.c	(revision 159179)
+++ fortran/intrinsic.c	(working copy)
@@ -4016,8 +4016,40 @@ gfc_convert_type_warn (gfc_expr *expr, g
     gfc_warning_now ("Extension: Conversion from %s to %s at %L",
 		     gfc_typename (&from_ts), gfc_typename (ts), &expr->where);
   else if (wflag && gfc_option.warn_conversion)
-    gfc_warning_now ("Conversion from %s to %s at %L",
-		     gfc_typename (&from_ts), gfc_typename (ts), &expr->where);
+    {
+      /* If the types are the same (but not LOGICAL), and if from-kind
+	 is larger than to-kind, this may indicate a loss of precision.
+	 The same holds for conversions from REAL to COMPLEX.  */
+      if (((from_ts.type == ts->type && from_ts.type != BT_LOGICAL)
+	    && from_ts.kind > ts->kind)
+	  || ((from_ts.type == BT_REAL && ts->type == BT_COMPLEX)
+	      && from_ts.kind > ts->kind))
+	gfc_warning_now ("Possible loss of precision in conversion "
+			 "from %s to %s at %L", gfc_typename (&from_ts),
+			 gfc_typename (ts), &expr->where);
+
+      /* If INTEGER is converted to REAL/COMPLEX, this is generally ok if
+	 the kind of the INTEGER value is less or equal to the kind of the
+	 REAL/COMPLEX one. Otherwise the value may not fit.
+	 Assignment of overly large integer constant also generates an
+	 overflow error on range checking. */
+      else if (from_ts.type == BT_INTEGER
+	       && (ts->type == BT_REAL || ts->type == BT_COMPLEX)
+	       && from_ts.kind > ts->kind)
+	gfc_warning_now ("Possible loss of digits in conversion "
+			 "from %s to %s at %L", gfc_typename (&from_ts),
+			 gfc_typename (ts), &expr->where);
+
+      /* If REAL/COMPLEX is converted to INTEGER, or COMPLEX is converted
+	 to REAL we almost certainly have a loss of digits, regardless of
+	 the respective kinds.  */
+      else if (((from_ts.type == BT_REAL || from_ts.type == BT_COMPLEX)
+		&& ts->type == BT_INTEGER)
+	       || (from_ts.type == BT_COMPLEX && ts->type == BT_REAL))
+	gfc_warning_now ("Likely loss of digits in conversion from"
+			 "%s to %s at %L", gfc_typename (&from_ts),
+			 gfc_typename (ts), &expr->where);
+    }
 
   /* Insert a pre-resolved function call to the right function.  */
   old_where = expr->where;
Index: testsuite/gfortran.dg/array_constructor_type_17.f03
===================================================================
--- testsuite/gfortran.dg/array_constructor_type_17.f03	(revision 159179)
+++ testsuite/gfortran.dg/array_constructor_type_17.f03	(working copy)
@@ -8,5 +8,5 @@ PROGRAM test
   IMPLICIT NONE
 
   INTEGER(KIND=4) :: arr(1)
-  arr = (/ INTEGER(KIND=4) :: HUGE(0_8) /) ! { dg-warning "Conversion from" }
+  arr = (/ INTEGER(KIND=4) :: HUGE(0_8) /) ! { dg-warning "conversion from" }
 END PROGRAM test

[-- Attachment #3: warn_conversion.f90 --]
[-- Type: text/x-fortran, Size: 1733 bytes --]

! { dg-do "compile" }
! { dg-options "-Wconversion" }

!
! PR fortran/27866 -improve -Wconversion
!
SUBROUTINE pr27866
  double precision :: d
  real   :: r
  d = 4d99
  r = d                 ! { dg-warning "conversion" }
END SUBROUTINE

SUBROUTINE pr27866c4
  real(kind=4)    :: a
  real(kind=8)    :: b
  integer(kind=1) :: i1
  integer(kind=4) :: i4
  i4 = 2.3              ! { dg-warning "conversion" }
  i1 = 500              ! { dg-error "overflow" }
                        ! { dg-warning "conversion" "" { target *-*-* } 20 }
  a = 2**26-1           ! assignment INTEGER(4) to REAL(4) - no warning
  b = 1d999             ! { dg-error "overflow" }

  a = i4                ! assignment INTEGER(4) to REAL(4) - no warning
  b = i4                ! assignment INTEGER(4) to REAL(8) - no warning
  i1 = i4               ! { dg-warning "conversion" }
  a = b                 ! { dg-warning "conversion" }
END SUBROUTINE


!
! PR fortran/35003 - spurious warning with -Wconversion
! Contributed by Brian Barnes <bcbarnes AT gmail DOT com>
!
SUBROUTINE pr35003
  IMPLICIT NONE
  integer(8) :: i, n
  n = 1_8

  do i = 1_8,n
  enddo
END SUBROUTINE


!
! PR fortran/42809 - Too much noise with -Wconversion
! Contributed by Harald Anlauf <anlauf AT gmx DOT de>
!
SUBROUTINE pr42809
  implicit none
  integer, parameter :: sp = kind (1.0)
  integer, parameter :: dp = kind (1.d0)
  real(sp)     :: s
  real(dp)     :: d
  complex(dp)  :: z

  s = 0                 ! assignment INTEGER(4) to REAL(4) - no warning
  d = s                 ! assignment REAL((8)) to REAL(4) - no warning
  z = (0, 1)            ! conversion INTEGER(4) to REAL(4),
                        ! assignment COMPLEX(4) to COMPLEX(8) - no warning
END SUBROUTINE

Re: [fortran, patch] reduce noise of -Wconversion

[Tobias Burnus]

Daniel Franke wrote:
> attached patch attempts to reduce amount of noise generated by -Wconversion.
>   

The changes make sense to me, however, I am wondering whether we need
some -Wconversion-extra option. The current -Wconversion option is very
verbose, but it allowed me to find bugs such as:

  double_var = sqrt(2.0)

where the RHS should be: "sqrt(2.0d0)". The current warning allows one
to find it (buried in tons of other messages) - but with the proposed
patch, this information is no longer available. I would like to get some
warning for (REAL/COMPLEX less precision) -> (REAL/COMPLEX higher
precision) back. I personally do not mind to have an extra option for
this. Thus, -Wall -Wextra finds both problems, but one can then be disabled.


I think regarding disabling, I like the new middle-end feature, which
prints the warning flag in squared brackets after the warning message
-fdiagnostics-show-option (enabled by default), e.g.
  foo.c:4:2: warning: 'j' is used uninitialized in this function
[-Wuninitialized]
and with -Werror:
  foo.c:4:2: error: 'j' is used uninitialized in this function
[-Werror=uninitialized]
And with -fno-diagnostics-show-option the [-W...] message is not printed.

Tobias

Re: [fortran, patch] reduce noise of -Wconversion

[Daniel Franke]

On Saturday 08 May 2010 20:17:25 Tobias Burnus wrote:
> Daniel Franke wrote:
> > attached patch attempts to reduce amount of noise generated by
> > -Wconversion.
> 
> The changes make sense to me, however, I am wondering whether we need
> some -Wconversion-extra option. The current -Wconversion option is very
> verbose, but it allowed me to find bugs such as:
> 
>   double_var = sqrt(2.0)
> 
> where the RHS should be: "sqrt(2.0d0)". The current warning allows one
> to find it (buried in tons of other messages) - but with the proposed
> patch, this information is no longer available. I would like to get some
> warning for (REAL/COMPLEX less precision) -> (REAL/COMPLEX higher
> precision) back. I personally do not mind to have an extra option for
> this. Thus, -Wall -Wextra finds both problems, but one can then be
> disabled.

Hi Tobias,

I think we should aim for the least amount of surprise. Lot's of people 
complained about the noisy behaviour and asked to get rid of the warnings for 
those perfectly valid conversions. If I read c-common.c (conversion_warning) 
correctly, then there's no warning for such less-to-higher precision in C.

Test ...

$> cat conversion.c
#include <math.h>
double f(float x) {
  return sqrt(x);
}
$> gcc-svn -Wall -Wconversion -c conversion.c
[no warning]

$ cat conversion.c
#include <math.h>
float f(double x) {
  return sqrt(x);
}
$ gcc-svn -Wall -Wconversion -c conversion.c 
conversion.c: In function 'f':
conversion.c:3:3: warning: conversion to 'float' from 'double' may alter its 
value [-Wconversion]



> I think regarding disabling, I like the new middle-end feature, which
> prints the warning flag in squared brackets after the warning message
> -fdiagnostics-show-option (enabled by default), e.g.
>   foo.c:4:2: warning: 'j' is used uninitialized in this function
> [-Wuninitialized]
> and with -Werror:
>   foo.c:4:2: error: 'j' is used uninitialized in this function
> [-Werror=uninitialized]
> And with -fno-diagnostics-show-option the [-W...] message is not printed.

While correct, I fail to see the relationship to the patch?!


Cheers

	Daniel

Re: [fortran, patch] reduce noise of -Wconversion

[Tobias Burnus]

Am 08.05.2010 20:52, schrieb Daniel Franke:
> On Saturday 08 May 2010 20:17:25 Tobias Burnus wrote:
>   
>> The current -Wconversion option is very verbose, but it allowed me to
>> find bugs such as:
>>   double_var = sqrt(2.0)
>>
>> where the RHS should be: "sqrt(2.0d0)".
>>     
> I think we should aim for the least amount of surprise. Lot's of people 
> complained about the noisy behaviour and asked to get rid of the warnings for 
> those perfectly valid conversions.

Thus, I was wondering:

>> I am wondering whether we need some -Wconversion-extra option.



>> I think regarding disabling, I like the new middle-end feature, which
>> prints the warning flag in squared brackets after the warning message
>>     
> While correct, I fail to see the relationship to the patch?!
>   

Well, one directly sees which warning comes from -Wconversion-extra and
which one from -Wconversion-extra, making it easier to disable only one
kind of conversion warnings without the other and without studying
in-depth the manual to find out that one can disable part of the warning.

Tobias

Re: [fortran, patch] reduce noise of -Wconversion

[Daniel Franke]

On Saturday 08 May 2010 21:06:56 Tobias Burnus wrote:
> >> I am wondering whether we need some -Wconversion-extra option.
> >> 
> >> I think regarding disabling, I like the new middle-end feature, which
> >> prints the warning flag in squared brackets after the warning message
> > 
> > While correct, I fail to see the relationship to the patch?!
> 
> Well, one directly sees which warning comes from -Wconversion-extra and
> which one from -Wconversion-extra, making it easier to disable only one
> kind of conversion warnings without the other and without studying
> in-depth the manual to find out that one can disable part of the warning.


Hmm, yes. Now it makes sense :)

Then I'd suggest to fully review the patch and apply it if acceptable.
Afterwards, open an enhancement PR to implement -Wconversion-all (?).

Combining the two gets messy quickly.

Cheers

	Daniel

Re: [fortran, patch] reduce noise of -Wconversion

[Tobias Burnus]

On 05/08/2010 07:15 PM, Daniel Franke wrote:
> attached patch attempts to reduce amount of noise generated by -Wconversion.
>
> Regression tested on i686-pc-linux-gnu.
> Ok for trunk?
>   

The patch is OK - thanks for going through the different cases and
making a sensible choice. I have created a follow up bug report: PR 44055.

Side remark: Regarding your "If I read c-common.c (conversion_warning)
correctly, then there's no warning for such less-to-higher precision in
C": Don't forget that in C floating-point literals are, by default,
double precision while in Fortran they are single. Thus, it is far more
likely to run into this problem in Fortran than in C.

Tobias

> gcc/fortran/:
> 2010-05-08  Daniel Franke  <franke.daniel@gmail.com>
>
> 	PR fortran/27866
> 	PR fortran/35003
> 	PR fortran/42809
> 	* intrinc.c (gfc_convert_type_warn): Be more dicriminative
> 	about conversion warnings.
>
> gcc/testsuite/:
> 2010-05-08  Daniel Franke  <franke.daniel@gmail.com>
>
> 	PR fortran/27866
> 	PR fortran/35003
> 	PR fortran/42809
> 	* gfortran.dg/array_constructor_type_17.f03: Updated match string.
> 	* gfortran.dg/warn_conversion.f90: New.
>