stdlib icon indicating copy to clipboard operation
stdlib copied to clipboard

Fix conversion warnings

Open Carltoffel opened this issue 4 years ago • 12 comments

What do you think about these changes?

pro fixes 12 warnings (the remaining 12 warnings should be easy to fix, too), which are ~ 60 lines to stderr

contra complexity of code (readability, maintainability, etc.)

Maybe there are better options to fix the warnings, please let me know.

Carltoffel avatar Sep 20 '21 08:09 Carltoffel

Obviously this is a subjective judgement. My feeling is that, if the original code is legal, then the extra code complexity isn't worth it.

It would be ideal if were compiler options to turn off this kind of warning. But I don't know of any.

gareth-nx avatar Sep 20 '21 09:09 gareth-nx

It would be ideal if were compiler options to turn off this kind of warning. But I don't know of any.

Usually it is the other way around: You can turn these warnings on with compiler options. Build systems tend to activate every warning available, because these warnings are useful (in general).

Carltoffel avatar Sep 20 '21 09:09 Carltoffel

Yes, I think for this reason gfortran supports turning warnings off, like (-Wno-implicit) as well as on (-Wimplicit). So you can use -Wall and just silence a few of them.

gareth-nx avatar Sep 20 '21 09:09 gareth-nx

It would be ideal if were compiler options to turn off this kind of warning. But I don't know of any.

We are getting a lot of false-positive and spurious warning from our current CMake setup. See https://github.com/fortran-lang/stdlib/issues/387 for further details.

awvwgk avatar Sep 20 '21 09:09 awvwgk

We are getting a lot of false-positive and spurious warning from our current CMake setup. See #387 for further details.

Are those conversion warnings I fixed really false-positive? There are implicit conversions happening, so I think these are true-positive warnings. The question is, should we:

  • Make the conversions explicit (verbose & complex code)
  • Hide all conversion warnings (might hide useful warnings)
  • Leave it as is (very verbose output, might also hide useful warnings)

Carltoffel avatar Sep 20 '21 09:09 Carltoffel

We are using -Wconversion-extra which basically flags every conversion even if it is safe to use.

https://github.com/fortran-lang/stdlib/blob/8bfcdf9fdf8d3d897eee76512ba5083ecab29f96/CMakeLists.txt#L27

awvwgk avatar Sep 20 '21 10:09 awvwgk

Okay, so do we want to leave the code as is and simply ignore the warnings?

Carltoffel avatar Sep 22 '21 06:09 Carltoffel

My suggestion would be to check our compiler warning options first and trim those down to the useful ones. Once we get rid of the 90% false positives, we fix the remaining warnings.

awvwgk avatar Sep 22 '21 06:09 awvwgk

I wonder if removing -Wconversion-extra is a good trade-off here?

gareth-nx avatar Sep 22 '21 07:09 gareth-nx

I did a quick test if we can leave out the kind= arguments, because this would make the code a little more readable. Unfortunately this doesn't solves the Conversion from COMPLEX(4) to COMPLEX(16) warnings. But maybe this will be an useful option with a different set of compiler flags.

Carltoffel avatar Sep 22 '21 08:09 Carltoffel

Most of them are due to specific compiler flags. I would first remove the useless ones, and then check the remaining warnings.

jvdp1 avatar Oct 02 '21 17:10 jvdp1

My suggestion would be to check our compiler warning options first and trim those down to the useful ones. Once we get rid of the 90% false positives, we fix the remaining warnings.

I agree with this. The next step would be to open a bug report with the compiler(s) that should not report false positives obfuscating the useful warnings. I don't think that we should bend stdlib to the needs of the compiler(s).

epagone avatar Oct 08 '21 10:10 epagone