A very pedantic pull request #105
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes several (extremely) minor issues in the source code which
were detected as nonconformat by the `-pedantic` flag.
None are serious, and none are likely to resolve any outstanding issues.
The main benefit is that it allows us to apply the `-pedantic` flag into
our testing, which can help to detect future issues.
The issues which have been fixed are described below.
* The `nX` edit descriptor must include the number of forward steps.
The implicit single step (`1x == x`) is a compiler extension.
* The no-advance line record write token `$` is a compiler extension
which is not described in the standard. Non-advancement is handled
with the `advance='no'` argument.
* Edit descriptors in format statements must be separated by commas,
even though many compilers will ignore missing commas if there is no
ambiguity.
* When line continuation tokens are applied to strings, they must appear
at both the end of the first line and the beginning of the subsequent
line. Most compilers do not require this second starting token.
* In function descriptions, if a variable used in the declaration of
another variable, such as an array index, then it must be declared
before any other variables refers to it.
For example, this is invalid:
```
function foo(i0, x)
real :: x(i0:)
integer :: i0
```
and `i0` must be declared before `x`.
Statement functions are obsolescent in the current language standard, and can be redefined as an internal function within a subprogram. This patch replaces the statement function `psi` (streamfunction) in `mixedlayer_restrat_general` as an explicit internal function.
The MOM_random generator used bit masks which were set with integer values. This is problematic for the sequence 0x8000 0000, because it must be set with a value of -2**31. In 4-byte integers, this is strictly not representable in Fortran, which requires symmetric signed domains for its variables. Since the upper bound is 2**31 - 1, the lower bound must be -2**31 + 1, which is larger than -2**31. Any value assigned to the 0x80000000 bit sequence is considered a noncompliant compiler extension. The current implementation seems to resolve this by using a kind=8 value (itself problematic, since 8-byte is not assured), but it still requires assigning this value to a 4-byte integer which cannot (strictly) represent the value. This patch averts the whole issue by explicitly setting the bits, and makes no reference to its integer value. It leaves the compiler to decide its interpretation. And since the variable is only used in bit operations, there is no ambiguity in behavior. Note that GCC 9 does not support BOZ conversion from z'80000000' to int, since it still expects BOZ literals to be within the bounds. This is why we use ibset() in place of a literal. Later GCC versions do not have this objection.
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #105 +/- ##
=========================================
Coverage 28.75% 28.75%
=========================================
Files 248 248
Lines 72970 72976 +6
=========================================
+ Hits 20979 20985 +6
Misses 51991 51991
Continue to review full report at Codecov.
|
Hallberg-NOAA
approved these changes
Apr 10, 2022
There was a problem hiding this comment.
I agree that all of these changes are useful, and will help to bring MOM6 into compliance with the most restrictive versions of the MOM6 standards. This PR has passed TC testing as well as pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/15196.
This was referenced May 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR resolves several issues which were detected by GCC's
-pedanticflag.Detailed explanations of changes are in the commit logs. A summary is provided below.
x) require an explicit step count. Single steps are now explicit. (x->1x).$) is not standard, and is replaced with theadvance='no'argument inwrite()MOM_mixedlayer_restrathas been redefined as an internal function.real :: i0must appear beforereal :: x(i0:)).2**-31to0x80000000is strictly incorrect (even if almost universally true), since variable ranges must be signed and symmetric, and this bit sequence is undefined forint32types. This has been replaced with explicit bit manipulation viaibset().None of these changes are particularly significant, but they will allow us to introduce the
-pedanticflag in our testing, which will help to preemptively detect issues on untested compilers and platforms.