Fix PGI warnings about intent for restart_CS#149
Merged
Conversation
- The PGI compiler was complaining about some `intent(in) :: CS`
in MOM_restart.F90. This was because of a line that changes the
state of data pointed to from within `CS`, but not `CS` itself:
CS%restart_field(n)%initialized = .true.
The strict interpretation is that `CS` is not modified because
`CS%restart_field` is a pointer to memory elsewhere. However, the
`intent(in)` indicates to the user/programmer that nothing changes
and since all arguments to the functions are `intent(in)` most entities,
including the PGI compiler, should be surprised that something changed
as a result of a passive "query" function. This strict interpretation
allows a devious hidden-change-of-state to occur.
- Changing the intent to `intent(inout)` has the consequence that the
new intent has to be propagated upwards through the code. And why should
a type be `intent(out)` for query functions?
- This commit removes offending lines that change the state. Apparently
we didn't need them!?
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #149 +/- ##
=========================================
Coverage 34.04% 34.05%
=========================================
Files 259 259
Lines 70138 70126 -12
Branches 12996 12984 -12
=========================================
- Hits 23880 23879 -1
+ Misses 41761 41753 -8
+ Partials 4497 4494 -3
Continue to review full report at Codecov.
|
Hallberg-NOAA
added a commit
to Hallberg-NOAA/MOM6
that referenced
this pull request
Jun 20, 2022
Added the overloaded interface set_initialized() to the MOM_restart module, to record that fields have been initialized, despite not appearing in a restart file. This will allow for a second call to set_initialized() after a call to query_initialized() to replicate the existing behavior of query_initialized() after MOM6 PR mom-ocean#149 (NOAA-GFDL#149) has been accepted. All answers are bitwise identical, but there is a new public interface.
Merged
Hallberg-NOAA
approved these changes
Jun 20, 2022
Member
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
I agree with these changes, provided we also consider the complementary pull request PR #152, which will recover the capability that is being eliminated.
I agree that query functions should not have the hidden side effect of modifying anything, but an explicit call to avoid repeating approximate initializations or recording that other dependent calculations can be done is still useful.
Member
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16040 ✔️ |
marshallward
pushed a commit
that referenced
this pull request
Jul 5, 2022
Added the overloaded interface set_initialized() to the MOM_restart module, to record that fields have been initialized, despite not appearing in a restart file. This will allow for a second call to set_initialized() after a call to query_initialized() to replicate the existing behavior of query_initialized() after MOM6 PR #149 (#149) has been accepted. All answers are bitwise identical, but there is a new public interface.
Hallberg-NOAA
added a commit
to Hallberg-NOAA/MOM6
that referenced
this pull request
Jul 18, 2022
Call set_initialized for variables after they are initialized, when their lack of initialization was detected via query_initialized. This commit reproduces the model's behavior prior to the removal of the code that set the initialized flag within query_initialized in NOAA-GFDL#149, and it completes the set of changes that was envisioned when set_initialized was added in NOAA-GFDL#152. All answers are bitwise identical.
marshallward
pushed a commit
that referenced
this pull request
Jul 26, 2022
Call set_initialized for variables after they are initialized, when their lack of initialization was detected via query_initialized. This commit reproduces the model's behavior prior to the removal of the code that set the initialized flag within query_initialized in #149, and it completes the set of changes that was envisioned when set_initialized was added in #152. All answers are bitwise identical.
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.
intent(in) :: CSin MOM_restart.F90. This was because of a line that changes the state of data pointed to from withinCS, but notCSitself:CS%restart_field(n)%initialized = .true.. The strict interpretation is thatCSis not modified becauseCS%restart_fieldis a pointer to memory elsewhere. However, theintent(in)indicates to the user/programmer that nothing changes and since all arguments to the functions areintent(in)most entities, including the PGI compiler, should be surprised that something changed as a result of a passive "query" function. This strict interpretation allows a devious(?) hidden-change-of-state to occur.intent(inout)has the consequence that the new intent has to be propagated upwards through the code. And why should a type beintent(out)for query functions?