Replace min/max variable names - #26
Merged
Merged
Conversation
Fortran has no way to reach an intrinsic that a local name has shadowed, so any scoping unit with a variable called min or max simply cannot call MIN() or MAX(). mpp_io has five such routines, and the shadowing is invisible at the point of failure: gfortran does not say "you shadowed an intrinsic", it says "Unclassifiable statement" on the line that tried to use it. Renamed, in mpp_io_mod and the fms_io/oda_tools code that calls it: mpp_write_meta_axis_r1d min -> valid_min mpp_write_meta_axis_i1d min -> valid_min mpp_write_meta_field min, max -> valid_min, valid_max mpp_modify_field_meta min, max -> valid_min, valid_max mpp_get_field_atts min, max -> valid_min, valid_max register_restart_axis_r1d min -> valid_min validtype %min, %max -> %valid_min, %valid_max fieldtype %min, %max -> %valid_min, %valid_max fms_io ax_type %min -> %valid_min valid_min and valid_max are not invented names: these variables exist to carry the CF attributes that the same routines already write as the string literals 'valid_min' and 'valid_max'. The declaration now matches the attribute it produces. The component renames are cosmetic -- a component does not shadow anything -- but validtype and fieldtype are PRIVATE-component types whose min/max are only reachable inside mpp_io_mod, so the rename is contained, and leaving field%min next to a valid_min dummy would be worse than either alone. API impact, audited rather than assumed. Dummy-argument names are visible to keyword callers. Every keyword caller of the affected routines in FMS is updated here -- 11 call sites in fms/fms_io.F90, fms/fms_io_unstructured_register_restart_axis.inc, fms/fms_io_unstructured_save_restart.inc and oda_tools/write_ocean_data.F90. MOM5 outside its vendored src/shared has *zero* keyword min=/max= callers of mpp_write_meta, mpp_modify_field_meta, mpp_get_field_atts or register_restart_axis, so re-vendoring src/shared is the whole of the MOM5 change. Positional callers are unaffected either way. Deliberately NOT renamed: the integer min variables in time_manager.F90, time_interp_external.F90 and oda_core_ecda.F90. Those hold minutes. They do shadow the intrinsic in their own scopes, but valid_min would be a lie; the right name there is 'minute' and it is a separate change. No behaviour change: this is a pure rename, and the CF attribute strings written to the files are untouched. This commit is self-contained and applies to the mom5 branch on its own. Applied on top of the parallel-netCDF series it also makes MAX() reachable in mpp_write_meta_field, which is what the chunking loop there had to work around by hand.
Companion to the valid_min/valid_max rename, covering the other family of
min shadowing in this tree. These variables hold minutes, so valid_min would
have been wrong for them; minutes is the name the surrounding code already
uses.
time_manager/time_manager.F90 date_to_string
program test (#ifdef test_time_manager)
time_interp/time_interp_external.F90 time_interp_external_3d
time_interp_external_0d
oda_tools/oda_core_ecda.F90 get_obs
Every one is an unpacking of get_date(time, yr, mo, day, hr, min, sec) into
locals. While the local is called min, the MIN intrinsic is unreachable in
that scoping unit, and gfortran reports an attempt to use it only as
"Unclassifiable statement" on the line that tried.
Unlike the valid_min/valid_max rename there is no API surface here at all:
all four declarations are plain locals, not dummy arguments, so no caller
anywhere can name them and nothing outside these three files can see the
change. The values are passed to get_date and increment_date positionally.
time_manager already uses minutes as the dummy-argument name in
increment_date, increment_date_private and decrement_date, so this makes the
module self-consistent rather than introducing a new convention. No program
unit that declares min also declares minutes, so there is no collision --
checked per scoping unit, not per file.
The time_manager 'program test' block sits behind #ifdef test_time_manager
and is not built by default; it was syntax-checked separately with
-Dtest_time_manager, clean both before and after this change.
No behaviour change: a pure rename of local variables, with no format string,
attribute or output text touched. The ' min=' label in format 11 stays as it
is -- that is display text, not an identifier.
This commit is independent of the parallel-netCDF series and applies to the
mom5 branch on its own.
Collaborator
Author
|
@dougiesquire @anton-seaice Will either of you be able to review this? |
manodeep
commented
Aug 20, 2026
Collaborator
Author
|
Thanks @dougiesquire |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
For metadata type variables
min/max->valid_min/valid_max; for time related variablesmin->minutesFixes #24