Skip to content

Conversation

@rhoneyager-tomorrow
Copy link
Contributor

@rhoneyager-tomorrow rhoneyager-tomorrow commented Apr 25, 2025

Description

Hey guys, I'm building jedi-bundle develop for the first time in a while, and am seeing lots of compiler warnings with icx / ifort. These compilers are being generous by passing much of this with warnings instead of raising errors, and I'd recommend tightening up the CI checks to catch these.

C++:

  • The oops CMake logic has been using classic Intel compiler flags where it should be using appropriate options for the new Clang-based compiler. That's because it is matching "Intel" against compiler ID, but "IntelLLVM" also matches.
  • You can't declare a runtime variable-length array on the stack. E.g. int field_vid[vars.size()] invokes undefined behavior. Prefer std::vector<int> field_vid(vars.size()).
  • Return values can't be references to temporary objects.
  • Under higher logging levels (e.g. -Wall and -warn all, there are many overloaded virtual function warnings. Essentially, base class functions are being masked (not overridden) with different parameters in derived classes, and that's messy.

Fortran:

  • Many functions are being referenced without a declaration (e.g. without extern and outside of a module).

Impact

At the very least, this fixes failures in test_util_signal_trap_fpe_div_by_zero and test_l95_getkf_cpp that I observed. Conversely, it exposes transient bugs in test_assimilation_etkf and test_qg_increment that should be diagnosed.

Checklist

  • I have performed a self-review of my own code
    • Yes, and I tested locally.
  • I have made corresponding changes to the documentation
    • No changes here.
  • I have run the unit tests before creating the PR

Copy link
Contributor

@fmahebert fmahebert left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @rhoneyager-tomorrow!

(sorry for slow review, I seem to never have gotten the notification about this one)

Copy link
Contributor

@srherbener srherbener left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these!

@fmahebert
Copy link
Contributor

I've opened a PR in JCSDA-internal from your branch, @rhoneyager-tomorrow.

@fmahebert
Copy link
Contributor

The PR in jcsda-internal was merged, so these changes should propagate to public. I'll close this PR now.

Thanks @rhoneyager-tomorrow for contributing these warning fixes!

@fmahebert fmahebert closed this Jun 18, 2025
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