Fixing some code warnings in utils #724
Conversation
tests/CMakeLists.txt
Outdated
| @@ -93,7 +93,7 @@ if(FTP_TEST_FILES) | |||
| blend.t19z.core.f001.co.grib2 | |||
| aqm.t12z.max_8hr_o3.227.grib2 | |||
| seaice.t00z.grb.grib2 | |||
| cmc_geavg.t12z.pgrb2a.0p50.f000 | |||
| #cmc_geavg.t12z.pgrb2a.0p50.f000 | |||
There was a problem hiding this comment.
Did you turn off a test file here?
There was a problem hiding this comment.
Yes, I will put it back. The CI was having some issues with downloading the test files and I assumed that it was the problem with one file, but I think it may have just been a temporary issue with the FTP site.
| @@ -24,7 +24,7 @@ if (BUILD_D) | |||
| if(CMAKE_C_COMPILER_ID MATCHES "^(Intel|IntelLLVM)$") | |||
| set_target_properties(copygb2 PROPERTIES COMPILE_FLAGS "-r8 -auto -convert big_endian -fpp") | |||
| elseif(CMAKE_C_COMPILER_ID MATCHES "^(GNU|Clang|AppleClang)$") | |||
| set_target_properties(copygb2 PROPERTIES COMPILE_FLAGS "-fdefault-real-8 -fconvert=big-endian -cpp") | |||
| set_target_properties(copygb2 PROPERTIES COMPILE_FLAGS "-fdefault-real-8 -fconvert=big-endian -cpp -frecursive") | |||
There was a problem hiding this comment.
It's not good to rely on compiler flags. What warnings does this new flag address?
There was a problem hiding this comment.
Here is the warning:
I could also increase the limit like the warning suggests?
| @@ -294,132 +294,132 @@ end subroutine frmt | |||
| !> | |||
| !> @author Stephen Gilbert @date 2010-09-08 | |||
| subroutine prvtime(ipdtn, ipdtmpl, listsec1, tabbrev) | |||
| implicit none | |||
There was a problem hiding this comment.
This reindent desn't look right. We use 4 spaces as the indent, not 7. Also don't use tabs.
Can you adjust your editor for these settings?
There was a problem hiding this comment.
The warning was for inconsistent indents which was caused by when I started combining some subroutines into one file. I followed the number of spaces from the rest of the files. Which part of the code are you referring to that looks incorrect? The updated code matches the rest of the files as far as I can tell.
|
OK, this is a great PR. I hate warnings in the code, they just train us programmers to ignore (new) warnings. So the code can just keep getting worse and worse, if we don't fix all warnings and then demand a warning-free build in the CI. But I'm a little worried about this PR, because our testing is not great (yet). Can you submit a new PR with the code re-indent, and the unused variable warnings resolved? Those are the easy ones which we can be sure is not breaking anything. Then, as a second pass, the code changes which remove parameters from the subroutine. That seems like a more serious edit and I would like to see that alone. Also we may wish to add more tests, and look at the code coverage report to make sure this code is being tested. |
|
Closing this one. See #728 |
Fixes NOAA-EMC/NCEPLIBS-grib_util#128
Fixes NOAA-EMC/NCEPLIBS-grib_util#132