toolchain: lint_source.py checks for integer(wp) and signed d-exponent literals (addresses #1485) - #1725
toolchain: lint_source.py checks for integer(wp) and signed d-exponent literals (addresses #1485)#1725BakaOverflow wants to merge 3 commits into
Conversation
|
@claude full review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1725 +/- ##
=======================================
Coverage 61.21% 61.21%
=======================================
Files 84 84
Lines 21603 21603
Branches 3189 3189
=======================================
Hits 13224 13224
Misses 6209 6209
Partials 2170 2170 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for this, and for flagging the two deviations explicitly in the description. Both calls were right; see the end of this comment. I verified the behavior rather than just reading the regexes. On current Two things to fix before this merges. 1. The mantissa only accepts one digit before the decimal point
The d-literal alternative is
There are two distinct defects here. The first is a false negative. The second is a misleading diagnostic. When the mantissa has several digits before the point, the match starts mid-number, so the error quotes a fragment. Both are fixed by allowing a full mantissa and adding r"(?<![A-Za-z0-9_.])(?:[0-9]+\.?[0-9]*|\.[0-9]+)[dD][-+]?[0-9]+(?![A-Za-z0-9_.])"I ran this variant over the whole Adding Please add 2.
|
Description
I'm a CodePath AI301 student contributing to MFC as part of an open-source capstone project.
Why. MFC enforces several source conventions only through code review, so violations keep
landing in
src/, and their failure modes are silent: ad-exponent literal such as5.0d-11hard-codes double precision and defeats MFC's build-time
wp/stpprecision switching, andinteger(wp)declares an integer with a real kind (a copy-paste slip).lint_source.pyhad too narrow a regex for the first and no check for the second, so neither was caught.
What. This PR covers two of the three gaps in #1485 and fixes the violations they surface:
Linter (
toolchain/mfc/lint_source.py):check_double_precision's d-literal regex from[0-9]d0to catch signed /multi-digit exponents (
5.0d-11,2.5d+3,1.0d12). I constrained it with identifierboundaries —
(?<![A-Za-z0-9_])[0-9]\.?[0-9]*[dD][-+]?[0-9]+(?![A-Za-z0-9_])— because thebare pattern in the issue also matches the
2d12inside identifiers likecart2d12_coords.check_integer_wp— flagsinteger(wp), suggests plaininteger.Both registered in
main(), with unit tests intest_lint_source.py.Source fixes:
m_bubbles_EE.fpp:integer(wp)→integerm_bubbles_EL.fpp:5.0d-11→5.0e-11_wpDeferred: the third gap —
check_stop_statements— is intentionally left out of this PR. Theexisting
stops can't simply becomes_mpi_abort(that routine is inm_mpi_common, whichalready
uses the modules thestops live in, so the import would be a circular dependency),which is why the
stopcleanup is tracked in #1483. I'll add thestopcheck in a follow-uppaired with #1483 so the check and the fixes land together.
@sbryngelson — two things I wanted to check with you. First, I tightened the d-literal regex from
the exact pattern in the issue by adding identifier boundaries, because the bare version also
matches names like
cart2d12_coords; I believe that only removes false positives, but wanted toflag the deviation. Second, I scoped this PR to the
integer(wp)and d-literal checks and left thestop/error stopcheck for a follow-up alongside #1483 — fixing thosestops cleanly runs intothe module-dependency cycle that issue already owns. Happy to pull the
stopcheck into this PRinstead if you'd prefer.
Addresses #1485. (Delivers two of the three requested checks; the
stop/error stopcheck is deferred to #1483 — see "Deferred" above.)Type of change
Testing
Local (rebased branch, gfortran 13.3.0, CPU):
python3 -m pytest toolchain/→ 368 passed (8 warnings, 4 subtests passed).python3 toolchain/mfc/lint_source.py→ exit 0 (clean tree)../mfc.sh precheck→ all 7 local gates pass../mfc.sh build→ exit 0 (all four targets).Full CI — intra-fork dry-run on the narrowed commit (
62a16ff2): 60 checks passed, 1 failed.Lagrange bubble cases included (the family an earlier full-scope attempt broke, now clean).
Coverage Test on CodeCovgreen; ✅ NVHPC GPU jobs green; ✅ lint / spell / convergence /FP-stability all green.
Build & Verify, at itsLinkcheck - Lycheestep — a[403]on a StackOverflow URLin
docs/documentation/docker.md, a file this PR does not touch and which is unchanged onmaster(pre-existing; not caused by this PR).Before / after — the gap this PR closes (verbatim linter output):
Checklist
GPU changes
integer-kind + literal-form changes only; no GPU logic; flagging for the CI matrix.