Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dummy argument aliasing issue #17

Merged
merged 2 commits into from
Jul 8, 2018

Conversation

billsacks
Copy link
Member

NAG version 6.2 catches a problem with passing the same variable to two
different arguments of a subroutine, one of which is intent(out). The
fix is to use a separate copy of the local variable for one of the
arguments.

Fixes #6

@billsacks billsacks requested a review from whlipscomb July 6, 2018 15:39
@billsacks
Copy link
Member Author

@whlipscomb - can you please review this and let me know if it looks okay? Then I'll need to rebase this to a common ancestor of the master and release branches and merge it into both.

@gunterl gunterl self-requested a review July 6, 2018 19:47
NAG version 6.2 catches a problem with passing the same variable to two
different arguments of a subroutine, one of which is intent(out). The
fix is to use a separate copy of the local variable for one of the
arguments.
@billsacks
Copy link
Member Author

I have rebased onto the release branch and force pushed. The diffs are the same as before.

As with the previous commit: NAG version 6.2 picked up some of these
issues.
@billsacks
Copy link
Member Author

@gunterl okay, you can go ahead and test these changes. I'm going to restart my own testing, too.

@billsacks billsacks changed the base branch from master to release-cism2.1 July 6, 2018 22:38
@gunterl
Copy link
Contributor

gunterl commented Jul 6, 2018 via email

@billsacks
Copy link
Member Author

I have completed my cism-in-cesm testing with these changes, and everything looks good: The NAG tests pass, and the cheyenne-intel/gnu tests pass and are bit-for-bit.

@gunterl and @whlipscomb once you sign off on these changes I'll bring the changes to master and the release branch. I want to do this in a very specific way to maintain the history I used for my testing in CESM, so please don't merge this yourself.

@billsacks
Copy link
Member Author

Sorry: While I'd like to wait for you two to sign off on this, I'm actually going to go ahead and merge this this weekend: The git steps I need to do are complicated enough that there's a good chance I'm going to mess something up if I wait to do this until I'm back from vacation. (This is complicated by the facts that (1) these fixes are needed ASAP for CESM testing, both on master and the release branch; (2) I'm trying to keep master, release and release-nohist all in sync.)

So you'll probably see this as merged soon (maybe tomorrow). I'll still welcome a review of these changes, though. If you see any problems, we can fix them in a separate commit.

@whlipscomb
Copy link
Contributor

@billsacks, I looked at the changes in parallel_mpi.F90 and glissade_transport.F90, and they seem fine. I also checked out escomp/master (after you merged these changes) and ran some standard standalone tests that (as I expected) had no problems. So I'm ready to sign off. Thanks for taking care of this fix.

@billsacks billsacks merged commit 6d5d112 into ESCOMP:release-cism2.1 Jul 8, 2018
@billsacks billsacks deleted the nag_fixes branch July 8, 2018 17:28
@billsacks billsacks changed the title DON'T MERGE YET: Fix dummy argument aliasing issue Fix dummy argument aliasing issue Jul 8, 2018
@gunterl
Copy link
Contributor

gunterl commented Jul 9, 2018 via email

billsacks added a commit that referenced this pull request Jun 23, 2021
Allow more characters in message string

This is a very simple PR to fix a problem seen by the Delft group. The
code was failing in glimmer_nc_readparams when the restart filename was
more than ~70 characters (e.g., because of a long absolute path). The
fix is to allow more characters in the 'message' string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants