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

Reduce GNU Compilation Warnings #1048

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

scrasmussen
Copy link
Member

@scrasmussen scrasmussen commented Jan 17, 2024

This PR fixes all but one set of warnings when building with GNU and using the standard level of warning flags.

The warnings fixed are summarized as:

  • Deleted Feature: do loop bounds must be integers

  • Integer to logical type conversion

  • Redefinition of CCPP macro

  • Function type mismatches from old NetCDF Fortran 77 interfaces

  • w3emc library warnings because they don't use generic interfaces this is now handled in PR Local w3emc module #1070

    The w3emc module commit is just a wrapper to the normal w3emc w3difdat and w3movdat functions but uses generic interfaces in a single file. This means the type warnings only are produced once when compiling the wrapper module, rather than in every file that calls w3difdat and w3movdat. This wrapper file module can easily be removed in the future if the w3emc library decides to move to generic interfaces.

This PR almost fulfills the GNU compiler part of the requirement "Code must compile without errors or warnings. Errors and warnings may not be suppressed, and the compiler warning level ("-W" options) must be at least the default one." as pointed out in UFS issue #1984. It also a step towards fulfulling the goal/requirement of no warnings from -Wall, as pointed out in #495.

@scrasmussen
Copy link
Member Author

This PR requires CCPP-SCM PR#424

@scrasmussen scrasmussen changed the title Recude GNU Compilation Warnings Reduce GNU Compilation Warnings Jan 17, 2024
Copy link
Collaborator

@SamuelTrahanNOAA SamuelTrahanNOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job tracking all of this down. I'm surprised the Intel compiler lets these errors through. The real-integer and integer-logical mismatches can be especially dangerous.

A word of warning. In earlier versions of CCPP w3 calls were crashing due to a 32 vs. 64 bit mismatch in some calls. The ufs-weather-model has regression tests to catch that specific problem. It may be wise to run those tests.

physics/scm_sfc_flux_spec.F90 Outdated Show resolved Hide resolved
physics/w3emc_wrapper.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@AnningCheng-NOAA AnningCheng-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look good to me.

@grantfirl
Copy link
Collaborator

I think that this is a good opportunity to eliminate the dependency of CCPP on NCEPlibs. See ufs-community#46

@grantfirl
Copy link
Collaborator

@scrasmussen Would it be too much of a pain to rebase this on top of the ufs-community/ccpp-physics ufs/dev branch? Same with #1049? The issue that this is addressing came from the UFS, so the solution should probably go in there first. If it's too much of an ask, we can merge into NCAR/main first. I'd rather see a different solution than the w3emc_wrapper, though, we should be able to eliminate the dependency on w3emc, which simplifies the changes here.

@scrasmussen
Copy link
Member Author

@scrasmussen Would it be too much of a pain to rebase this on top of the ufs-community/ccpp-physics ufs/dev branch? Same with #1049? The issue that this is addressing came from the UFS, so the solution should probably go in there first. If it's too much of an ask, we can merge into NCAR/main first. I'd rather see a different solution than the w3emc_wrapper, though, we should be able to eliminate the dependency on w3emc, which simplifies the changes here.

@grantfirl sorry, had missed this message. Not too much of a pain at all, will get these rebased soon! Also, will have an incoming PR that brings in the functions used from the w3emc and puts them in a module that the physics will build. There then won't be a w3emc dependency and it uses generic interfaces so the warnings from the w3emc functions will all be gone.

@scrasmussen scrasmussen force-pushed the compiler-warning-reduction/w3emc-and-others branch from 5a69249 to effeeb8 Compare May 9, 2024 18:40
@scrasmussen
Copy link
Member Author

@grantfirl this PR has now been rebased to the latest on main

@grantfirl
Copy link
Collaborator

@scrasmussen Sorry, I wasn't clear. In addition to being rebased, this PR should be targeted to ufs-community:ufs/dev.

@scrasmussen scrasmussen force-pushed the compiler-warning-reduction/w3emc-and-others branch from effeeb8 to e6d218e Compare December 10, 2024 23:04
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.

7 participants