Fix debug build warnings: intent(out) init, unused vars, dead code, float comparisons, precision tolerance#1341
Fix debug build warnings: intent(out) init, unused vars, dead code, float comparisons, precision tolerance#1341sbryngelson wants to merge 12 commits intomasterfrom
Conversation
|
Claude Code Review Incremental review from: a8e1ea8 New findings since last Claude review:
|
📝 WalkthroughWalkthroughThis change introduces a new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6b24091-bf9c-42f2-bdb6-2337efce7132
📒 Files selected for processing (16)
.github/workflows/common/build-warnings.sh.github/workflows/test.ymlCMakeLists.txtsrc/common/m_helper.fppsrc/common/m_mpi_common.fppsrc/common/m_variables_conversion.fppsrc/post_process/m_data_output.fppsrc/pre_process/m_icpp_patches.fppsrc/simulation/m_bubbles_EL.fppsrc/simulation/m_compute_cbc.fppsrc/simulation/m_compute_levelset.fppsrc/simulation/m_start_up.fpptoolchain/mfc/build.pytoolchain/mfc/cli/argparse_gen.pytoolchain/mfc/cli/docs_gen.pytoolchain/mfc/state.py
💤 Files with no reviewable changes (1)
- src/simulation/m_compute_cbc.fpp
| # RelDebug: a lighter debug mode for CI. Compiler-specific blocks below add the | ||
| # actual flags; these defaults just tell CMake it is a recognised build type. | ||
| set(CMAKE_C_FLAGS_RELDEBUG "-g" CACHE STRING "") | ||
| set(CMAKE_CXX_FLAGS_RELDEBUG "-g" CACHE STRING "") | ||
| set(CMAKE_Fortran_FLAGS_RELDEBUG "-g" CACHE STRING "") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'CMAKE_CONFIGURATION_TYPES|CONFIG:RelDebug|CMAKE_BUILD_TYPE STREQUAL "RelDebug"|--config[^[:alnum:]]*RelDebug|RelDebug' \
CMakeLists.txt toolchain/mfcRepository: MFlowCode/MFC
Length of output: 3855
RelDebug build type is implemented only for single-config generators.
The CMAKE_*_FLAGS_RELDEBUG flags are defined here, but throughout CMakeLists.txt, RelDebug configuration is selected via if (CMAKE_BUILD_TYPE STREQUAL "RelDebug") checks. This works only for single-config generators (Make, Ninja); multi-config generators (Visual Studio, Xcode, Ninja Multi-Config) do not set CMAKE_BUILD_TYPE and instead require:
- Registration in
CMAKE_CONFIGURATION_TYPES - Config-scoped settings via
$<CONFIG:RelDebug>generator expressions
Currently, RelDebug will not be available or will be partially non-functional for multi-config generators.
| # Set build type (Debug, RelDebug, or Release). | ||
| # See: https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html | ||
| f"-DCMAKE_BUILD_TYPE={'Debug' if ARG('debug') else 'Release'}", | ||
| f"-DCMAKE_BUILD_TYPE={'Debug' if ARG('debug') else 'RelDebug' if ARG('reldebug') else 'Release'}", | ||
| # Used by FIND_PACKAGE (/FindXXX) to search for packages, with the |
There was a problem hiding this comment.
Make conflicting build modes an error.
--debug and --reldebug can both be true today, and these ternaries quietly collapse that state to Debug. Reject the combination once so configure/build/reporting stay unambiguous.
💡 Suggested fix
+ if ARG("debug") and ARG("reldebug"):
+ raise MFCException("`--debug` and `--reldebug` are mutually exclusive.")
+ build_type = "Debug" if ARG("debug") else "RelDebug" if ARG("reldebug") else "Release"
+
flags: list = self.flags.copy() + [
@@
- f"-DCMAKE_BUILD_TYPE={'Debug' if ARG('debug') else 'RelDebug' if ARG('reldebug') else 'Release'}",
+ f"-DCMAKE_BUILD_TYPE={build_type}",
@@
- "Debug" if ARG("debug") else "RelDebug" if ARG("reldebug") else "Release",
+ build_type,Also applies to: 458-468
sbryngelson
force-pushed
the
fix-debug-warnings
branch
3 times, most recently
from
f0672e9 to
b1351c8
Compare
Initialize intent(out) arguments unconditionally before #ifdef blocks: - m_mpi_common.fpp: icfl_max_glb, vcfl_max_glb, Rc_min_glb - m_variables_conversion.fpp: rho_K, gamma_K, pi_inf_K, qv_K, Re_K, G_K - m_mpi_common.fpp: change intent(out) to intent(inout) for aliased allreduce Remove unused local variables verified across all build targets.
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
b1351c8 to
8f98be8
Compare
Remove dead CCFL variables from m_data_output.fpp (ccfl_sf, ccfl_max_loc, ccfl_max_glb, ccfl_max). Remove dead gradient magnitude variables from m_rhs.fpp (gm_alpha_qp, gm_alphaL_n, gm_alphaR_n, nbub) including their GPU_DECLARE and ALLOCATE. Remove 13 unused module-level variables from m_ib_patches.fpp and 7 unused local variables from its subroutines. Remove unused idist/odist from m_fftw.fpp and dest from m_time_steppers.fpp.
Remove unused locals in m_boundary_common.fpp (status, i, offset), m_model.fpp (theta, ndot, norm_mag, c, total_vertices, eta, max_iv1/2), m_data_input.f90 (delx, dely, delz), and m_time_steppers.fpp (gm_alpha_qp). Remove dead module-level bubrs_qbmm from m_qbmm.fpp (allocated and GPU-updated but never read by any code).
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
6131df3 to
6594f4d
Compare
Dead subroutines (zero call sites, verified case-insensitive across entire repo including Fypp includes, hardcoded IC dispatch tables, and case.fpp): - s_convert_cylindrical_to_spherical_coord in m_ib_patches.fpp and m_icpp_patches.fpp - s_compute_fd_divergence in m_finite_differences.fpp - s_solve_linear_system in post_process/m_derived_variables.fpp Dead stores (assigned but never read): - pres_mag, loc in m_assign_variables.fpp - M11 in simulation/m_data_output.fpp - flg in post_process/m_derived_variables.fpp - str_MOK, NVARS_MOK in pre_process and simulation m_start_up.fpp Unused variables: - my_divu in m_bubbles_EE.fpp (declared and in GPU private clause but never used) - theta1, theta2, En_tot, dirname, dir_exists in post_process/m_start_up.fpp - i in m_perturbation.fpp s_prng - t_step_ib_dir in post_process/m_data_input.f90
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
5fbc676 to
42ac3ce
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1341 +/- ##
==========================================
+ Coverage 64.67% 64.85% +0.17%
==========================================
Files 70 70
Lines 18251 18228 -23
Branches 1504 1506 +2
==========================================
+ Hits 11804 11821 +17
+ Misses 5492 5446 -46
- Partials 955 961 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- m_ibm.fpp: Re(1) /= 0._wp -> Re(1) > 0._wp (matches convention in m_global_parameters.fpp where Re is checked with > 0) - m_ibm.fpp: sqrt(sum(axis**2)) == 0 -> < sgm_eps (floating-point magnitude should not be compared with exact equality) - ExtrusionHardcodedIC.fpp: dummy_x == x0 -> f_approx_equal (coordinates read from files should not be compared with exact equality)
|
If @wilfonba @DimAdam-01 @danieljvickers could double check this I would appreciate it |
Single precision (wp=real(4)) has ~7 decimal digits of precision, so a default tolerance of 1e-10 is below machine epsilon (~1.2e-7) and f_approx_equal would almost never return true. Use 1e-6 for single precision builds and keep 1e-10 for double precision. The wp == single_precision comparison uses integer parameter constants that the compiler resolves at compile time, so there is no runtime cost.
sbryngelson
changed the title
…ve dead store - Remove sph_phi from m_ib_patches.fpp and m_icpp_patches.fpp (only user was s_convert_cylindrical_to_spherical_coord, removed in prior commit) - Fix #ifdef DEBUG to #ifdef MFC_DEBUG in m_data_output.fpp and m_variables_conversion.fpp (bare DEBUG is never defined, MFC_DEBUG is) - Remove dead igr assignment in toolchain/mfc/case.py (overwritten on line 360 without being read)
|
@sbryngelson Looks good to me |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
2 participants
Summary
Fixes compiler warnings from
--debug/--reldebuggfortran builds. All changes verified across all build targets (pre_process, simulation, post_process) and all#ifdefpaths.Intent(out) initialization
intent(out)arguments unconditionally before#ifdef MFC_SIMULATIONblocks inm_mpi_common.fppandm_variables_conversion.fppintent(out)tointent(inout)ins_mpi_allreduce_vectors_sumfor aliased callsUnused variables and dead code
m_rhs.fpp(gm_alpha_qp,gm_alphaL_n,gm_alphaR_n,nbub)m_data_output.fppm_ib_patches.fppbubrs_qbmmfromm_qbmm.fpppres_mag,loc,M11,flg,str_MOK,NVARS_MOK)Dead subroutines
s_convert_cylindrical_to_spherical_coord(m_ib_patches.fpp, m_icpp_patches.fpp)s_compute_fd_divergence(m_finite_differences.fpp)s_solve_linear_system(post_process/m_derived_variables.fpp)Floating-point equality fixes
m_ibm.fpp:Re(1) /= 0._wp→Re(1) > 0._wp(matches codebase convention)m_ibm.fpp:sqrt(sum(axis**2)) == 0→< sgm_epsExtrusionHardcodedIC.fpp:dummy_x == x0→f_approx_equalPrecision-aware tolerance
f_approx_equal/f_approx_in_array: default tolerance1e-6for single precision,1e-10for double (single precision epsilon is ~1.2e-7, so 1e-10 was below machine precision)Test plan
./mfc.sh precheck -j 8passes./mfc.sh build -j 8passes (all targets, gfortran)./mfc.sh test --reldebug -j 8 -% 5— 27/27 pass