lapack
lapack copied to clipboard
Reference-LAPACK
Define conversions explicitly, and use more rigorous flags in the CI
PR #702 fixed a problem that could be avoided by using more strict flags in the CI. This PR:
- Applies explicit type casts all over the library (not including tests for now).
- Use more strict flags in the CI.
I still couldn't use -Werror=lto-type-mismatch, as suggested in https://github.com/Reference-LAPACK/lapack/pull/702#issuecomment-1210683461, because the tests still have type mismatch and/or implicit type casts.
The compiler will do the correct cast in most (all) cases. The point is separate compiler warnings that can really help identify a bug from the type casts that make sense. After those changes, it is easier to use -Wall and find true issues in the code.
Sorry for the work I caused with my suggestion :smile:
I also think the explicit conversions are a good improvement. I am not quite clear on how these changes relate to LTO though, to me it seems like it does not really belong to this commit? I'm not sure how much value there is in enabling LTO in the CI without -Werror=lto-type-mismatch in the FFLAGS. But I suppose it does not really hurt to enable it here already (though it does considerably increase link-time).
The additional errors from -Werror=lto-type-mismatch that I unfortunately missed in #702 seem to be exclusively in the testing code for me. This commit fixes them for me: https://github.com/mjacobse/lapack/commit/35dd277cdd1053324e2f6848393252740607ee29 (edit: actually, more stuff seems to come up when compiling CBLAS too...)
After rereading your message I understand that you have already tracked these down though @weslleyspereira ? Do you intend to create another pull request for those?
Personally I am also a bit cautious with the general -Werror for all warnings instead of a specific one. A compiler update that introduces a new warning into a default set may easily break builds that way. But I suppose in the CI that might even be a good thing?
Sorry for the work I caused with my suggestion smile
No problem at all. This is time we spend now to avoid problems in future.
I also think the explicit conversions are a good improvement. I am not quite clear on how these changes relate to LTO though, to me it seems like it does not really belong to this commit? I'm not sure how much value there is in enabling LTO in the CI without
-Werror=lto-type-mismatchin theFFLAGS. But I suppose it does not really hurt to enable it here already (though it does considerably increase link-time).
When you do not use -Werror=lto-type-mismatch but -Wall are active, you still obtain warnings [-Wlto-type-mismatch]. This PR fixes all warnings [-Wconversion] I could find (excluding the ones in the test suite. I forgot to say that. It is helpful to have those warnings.
The additional errors from
-Werror=lto-type-mismatchthat I unfortunately missed in #702 seem to be exclusively in the testing code for me. This commit fixes them for me: mjacobse@35dd277 After rereading your message I understand that you have already tracked these down though @weslleyspereira ? Do you intend to create another pull request for those?
Great! Can I merge your commit and add it to this branch? Or maybe you want to propose a PR. Thanks.
No, I have just tracked the issues related to [-Wconversion]. I am sorry my initial message was not complete.
Personally I am also a bit cautious with the general
-Werrorfor all warnings instead of a specific one. A compiler update that introduces a new warning into a default set may easily break builds that way. But I suppose in the CI that might even be a good thing?
Yeah... -Werror is too much, and may cause problems Thanks. It was just for my testing purposes. I will come back to -Wall and then we can promote some warnings to errors in the CI. It is a good thing that the CI is more strict in my view.
Sure, here is the PR: https://github.com/Reference-LAPACK/lapack/pull/706
The CBLAS (and probably also LAPACKE) stuff will need more work I'm afraid. On a first glance it seems like the recent BLAS_FORTRAN_STRLEN_END addition might have to be sprinkled all over the CBLAS test code too. But maybe there are better solutions.
Codecov Report
Merging #703 (636150e) into master (cb8d38c) will not change coverage. The diff coverage is
0.00%.
:exclamation: Current head 636150e differs from pull request most recent head 9f65664. Consider uploading reports for the commit 9f65664 to get more accurate results
@@ Coverage Diff @@
## master #703 +/- ##
=======================================
Coverage 0.00% 0.00%
=======================================
Files 1894 1894
Lines 184078 184078
=======================================
Misses 184078 184078
| Impacted Files | Coverage Δ | |
|---|---|---|
| SRC/cgebak.f | 0.00% <0.00%> (ø) |
|
| SRC/cgees.f | 0.00% <0.00%> (ø) |
|
| SRC/cgeesx.f | 0.00% <0.00%> (ø) |
|
| SRC/cgejsv.f | 0.00% <0.00%> (ø) |
|
| SRC/cggbak.f | 0.00% <0.00%> (ø) |
|
| SRC/cggbal.f | 0.00% <0.00%> (ø) |
|
| SRC/cggglm.f | 0.00% <0.00%> (ø) |
|
| SRC/cgghd3.f | 0.00% <0.00%> (ø) |
|
| SRC/cgglse.f | 0.00% <0.00%> (ø) |
|
| SRC/cggqrf.f | 0.00% <0.00%> (ø) |
|
| ... and 57 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Notes:
Commit e203d67 has:
-Wlto-type-mismatch: 13
-Wconversion: 54
-Wsurprising: 2
-Winteger-division: 4
-Wintrinsic-shadow: 1
-Wunused-dummy-argument: 173
-Wunused-variable: 26
-Wunused-label: 2
- The warnings related to
-Wlto-type-mismatchare addressed in #706. - The warnings
-Wconversionare related to the tests.
Last commits:
- I removed all warnings related to Implicit conversion on the Fortran code, and activate the flag
-Werror=conversionin the CI - The CI using Makefile uses Link Time Optimization (LTO) so we can tests the library in this configuration.