Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Dominique d'Humières]

As said in bugzilla, I am against this change. If you want to use -Wconversion-extra, just add it to your favorite options.

-Wconversion-extra is extremely noisy and -Wextra has been stable for some years. IMO we cannot afford to have people complaining about the change.

If you really want a synthetic option, why not a new one -Wnoisy (or -Weally-all, …) which implies '-Wall -Wextra  -Wconversion-extra …’?

Dominique

PS. Who wants

Warning: Conversion from 'REAL(4)' to 'REAL(8)' at (1) [-Wconversion-extra]

even if may allow to detect things such as ‘pi8=acos(-1.0)’?

Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Thomas Koenig]

Am 18.09.2017 um 11:50 schrieb Dominique d'Humières:
> Warning: Conversion from 'REAL(4)' to 'REAL(8)' at (1) [-Wconversion-extra]

Not me (not in the general case)

> even if may allow to detect things such as ‘pi8=acos(-1.0)’?

This one would be interesting to catch (even with -Wall), although
it would be hard to avoid false positives.

I agree with Dominique that enabling -Wconversion-extra
the way it is now would be too noisy.

Are there warnings currently enabled with -Wconversion-extra
which would be useful in -Wextra (or even ones currently not in
-Wconversion-extra) without having too many false positives?

r8 = 1./3. could be one example, r8=acos(-1.0) another.  Something
like "it must have been calculated with a formula and, if evaluated
in the precision of the lhs, gives a different result from the one
if the user had specified it with the correct precision".

We don't do that yet, but I think it could be useful even with -Wall,
and certainly with -Wextra.

What do you think?

Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Dominique d'Humières]

> Le 19 sept. 2017 à 21:59, Thomas Koenig <tkoenig@netcologne.de> a écrit :
> 
> Am 18.09.2017 um 11:50 schrieb Dominique d'Humières:
>> Warning: Conversion from 'REAL(4)' to 'REAL(8)' at (1) [-Wconversion-extra]
> 
> Not me (not in the general case)
> 
>> even if may allow to detect things such as ‘pi8=acos(-1.0)’?
> 
> This one would be interesting to catch (even with -Wall), although
> it would be hard to avoid false positives.
> 
> I agree with Dominique that enabling -Wconversion-extra
> the way it is now would be too noisy.
> 
> Are there warnings currently enabled with -Wconversion-extra
> which would be useful in -Wextra (or even ones currently not in
> -Wconversion-extra) without having too many false positives?
> 
> r8 = 1./3. could be one example, r8=acos(-1.0) another.  Something
> like "it must have been calculated with a formula and, if evaluated
> in the precision of the lhs, gives a different result from the one
> if the user had specified it with the correct precision".
> 
> We don't do that yet, but I think it could be useful even with -Wall,
> and certainly with -Wextra.
> 
> What do you think?

Is it really worth the trouble?

I am really upset by the time spent on warning at the expense of more serious problems.

Dominique

Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Steve Kargl]

On Tue, Sep 19, 2017 at 10:26:52PM +0200, Dominique d'Humières wrote:
> 
> I am really upset by the time spent on warning at the expense
> of more serious problems.
> 

https://tinyurl.com/y6wunnk9

https://gcc.gnu.org/ml/fortran/2017-09/msg00065.html

or this is a consequence

https://gcc.gnu.org/ml/gcc-bugs/2017-09/msg01987.html
https://gcc.gnu.org/ml/gcc-bugs/2017-09/msg01866.html
https://gcc.gnu.org/ml/gcc-bugs/2017-09/msg01865.html
https://gcc.gnu.org/ml/gcc-bugs/2017-09/msg01862.html
https://gcc.gnu.org/ml/gcc-bugs/2017-09/msg01854.html
https://gcc.gnu.org/ml/gcc-bugs/2017-09/msg01850.html
...
https://gcc.gnu.org/ml/gcc-bugs/2017-09/msg01842.html

-- 
Steve

Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Janus Weil]

> As a sidenote, I made an observation that is not directly related to
> the patch: The second warning in the test case, on "i4 = i8" shows
> [-Wconversion] when compiled with -Wall, but [-Wconversion-extra] when
> compiled with -Wextra. Does anyone understand how that inconsistency
> comes about?

Btw, this is probably due to constructs like:

if ((warn_conversion || warn_conversion_extra) ...

which occur in a few places in arith.c. In this sense, the
documentation is apparently not fully correct, claiming that
-Wconversion-extra does not imply -Wconversion? It probably does not
do so for all warnings, but for some of them it does apparently?

Maybe it would be clearer to just make -Wconversion-extra imply
-Wconversion (and document that), in order to avoid confusion?

Cheers,
Janus

Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Janus Weil]

Hi Thomas,

>> here is a small patch that enables -Wconversion-extra with -Wextra and
>> updates the documentation.
>
> I grepped for warn_conversion_extra and found 14 occurrences in the
> gfortran source tree.
>
> Are we sure we want to enable each of these warnings with -Wextra?

I'd say: Yes, why not. They're all similar in style, it seems. Which
one are you worried about, in particular?

AFAICS, these warnings will essentially be triggered on compile-time
constants, for which the compiler knows that they can be converted
e.g. from real to integer without change in value. For such constants,
I guess it is easy (and advisable) to change them.

If you're writing '2.0' and assign it to an integer, then you probably
just mean '2'. I'd say the warning is justified for such cases.

Cheers,
Janus

Re: [Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Thomas Koenig]

Hi Janus,

> here is a small patch that enables -Wconversion-extra with -Wextra and
> updates the documentation.

I grepped for warn_conversion_extra and found 14 occurrences in the
gfortran source tree.

Are we sure we want to enable each of these warnings with -Wextra?

Regards

	Thomas

[Patch, Fortran] PR 82018: -Wextra should imply -Wconversion-extra

[Janus Weil]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

Hi all,

here is a small patch that enables -Wconversion-extra with -Wextra and
updates the documentation.

Up to gfortran 5, the warning in the test case was enabled by
-Wconversion, and thus with -Wall. That was changed by Thomas in
r224190, which makes complete sense to me.

However, I think -Wconversion-extra should be implied by -Wextra (I'm
sure not everyone wants to see such messages, but I think they're
still useful).

The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?

As a sidenote, I made an observation that is not directly related to
the patch: The second warning in the test case, on "i4 = i8" shows
[-Wconversion] when compiled with -Wall, but [-Wconversion-extra] when
compiled with -Wextra. Does anyone understand how that inconsistency
comes about?

Cheers,
Janus



2017-09-16  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/82018
    * lang.opt: Turn on -Wconversion-extra with -Wextra.
    * invoke.texi: Update documentation of -Wconversion-extra and -Wextra
    and mention both in the list of Error and Warning Options.

2017-09-16  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/82018
    * gfortran.dg/wextra_2.f90: New test case.

[-- Attachment #2: pr82018.diff --]
[-- Type: text/plain, Size: 2502 bytes --]

Index: gcc/fortran/invoke.texi
===================================================================
--- gcc/fortran/invoke.texi	(revision 252872)
+++ gcc/fortran/invoke.texi	(working copy)
@@ -144,10 +144,10 @@ by type.  Explanations are in the following sectio
 @item Error and Warning Options
 @xref{Error and Warning Options,,Options to request or suppress errors
 and warnings}.
-@gccoptlist{-Waliasing -Wall -Wampersand -Wargument-mismatch -Warray-bounds
--Wc-binding-type -Wcharacter-truncation @gol
--Wconversion -Wfunction-elimination -Wimplicit-interface @gol
--Wimplicit-procedure -Wintrinsic-shadow -Wuse-without-only -Wintrinsics-std @gol
+@gccoptlist{-Waliasing -Wall -Wampersand -Wargument-mismatch -Warray-bounds @gol
+-Wc-binding-type -Wcharacter-truncation -Wconversion -Wconversion-extra @gol
+-Wextra -Wfunction-elimination -Wimplicit-interface -Wimplicit-procedure @gol
+-Wintrinsic-shadow -Wuse-without-only -Wintrinsics-std @gol
 -Wline-truncation -Wno-align-commons -Wno-tabs -Wreal-q-constant @gol
 -Wsurprising -Wunderflow -Wunused-parameter -Wrealloc-lhs -Wrealloc-lhs-all @gol
 -Wtarget-lifetime -fmax-errors=@var{n} -fsyntax-only -pedantic -pedantic-errors
@@ -884,7 +884,7 @@ the expression after conversion. Implied by @optio
 @cindex warnings, conversion
 @cindex conversion
 Warn about implicit conversions between different types and kinds. This
-option does @emph{not} imply @option{-Wconversion}.
+option does @emph{not} imply @option{-Wconversion}. Implied by @option{-Wextra}.
 
 @item -Wextra
 @opindex @code{Wextra}
@@ -891,8 +891,8 @@ Warn about implicit conversions between different
 @cindex extra warnings
 @cindex warnings, extra
 Enables some warning options for usages of language features which
-may be problematic. This currently includes @option{-Wcompare-reals}
-and @option{-Wunused-parameter}.
+may be problematic. This currently includes @option{-Wcompare-reals},
+@option{-Wunused-parameter} and @option{-Wconversion-extra}.
 
 @item -Wimplicit-interface
 @opindex @code{Wimplicit-interface}
Index: gcc/fortran/lang.opt
===================================================================
--- gcc/fortran/lang.opt	(revision 252872)
+++ gcc/fortran/lang.opt	(working copy)
@@ -234,7 +234,7 @@ Fortran Var(warn_conversion) Warning LangEnabledBy
 ; Documented in C
 
 Wconversion-extra
-Fortran Var(warn_conversion_extra) Warning
+Fortran Var(warn_conversion_extra) Warning LangEnabledBy(Fortran,Wextra)
 Warn about most implicit conversions.
 
 Wextra

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

! { dg-do compile }
! { dg-options "-Wall -Wextra" }
!
! PR 82018: -Wextra should imply -Wconversion-extra
!
! Contributed by Janus Weil <janus@gcc.gnu.org>

implicit none
integer(kind=4) :: i4
integer(kind=8) :: i8
i8 = 2.0   ! { dg-warning "Conversion" }
i4 = i8    ! { dg-warning "Possible change of value in conversion" }
end