Fix REAL*8 statement-function declarations and tab warnings in r10vol1.f / r10tap.f (no numerical change) - #18
Open
d-diaz wants to merge 1 commit into
Conversation
…inators in r10vol1.f and r10tap.f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Clears 102 gfortran
-Wallwarnings in two Region 10 files —r10vol1.f78 → 2 andr10tap.f26 → 0 — with no change to computed results. The compiler emits byte-identical machine code before and after, at both-O0and-O2, so this is verifiably a diagnostics-only change.Most of it comes from one line per file: seven statement functions are declared
REAL*8although their bodies are entirely single-precision, so every use of them warned about a narrowing that the compiler was already performing.All line numbers below refer to the pre-change files, so they match what a reviewer sees on the left side of the diff. The two block-
DOconversions shift everything afterr10vol1.f:131down by one and everything after:271down by one more.The warning
Verbatim, from
gfortran -Wall -c r10vol1.fon gfortran 13.3.0:Counts before this change:
r10vol1.fr10tap.fREAL(8)→REAL(4)conversion[-Wconversion]REAL(4)→INTEGER(4)conversion[-Wconversion][-Wtabs][-Wunused-label]DOtermination[-Wunused-dummy-argument]What triggers it
r10vol1.f:534andr10tap.f:25declare seven statement functions — the Region 10 taper and bark-thickness equations — asREAL*8:Every dummy argument of those seven (
RH,RH32,RH40,D,H,DBHOB) is declared defaultREALon the three preceding lines, and every literal in every body is a default-real constant — there is nod0anywhere in them. For example:A statement function's expression is evaluated in its own operand types, and the value is converted to the declared result type afterwards — the same rule that makes
REAL*8 X; X = A*BevaluateA*Bin single precision whenAandBareREAL*4. So theREAL*8declaration widened an already-rounded single-precision value to double, and every one of the 33 uses immediately narrowed it back at aREAL(4)assignment target:That round trip is what
-Wconversionreports. The declaration is the actual defect; the warnings are its symptom.The fix
Before (
r10vol1.f:534, and identicallyr10tap.f:25):After:
REAL BKAC,DVA,BKWR,DVR,BB,DD2MI,DVREDAThis computes the same result because the expressions were always single-precision. The declaration only controlled the type of an intermediate that was immediately converted back — every one of the 33 call sites is a bare assignment into a
REAL(4)target, with no site where the result feeds a wider expression. Removing the round trip removes nothing else.Also in this change
Two implicit real→integer conversions in
r10vol1.fmade explicit, preserving the existing truncate-toward-zero behaviour:Line 129, two statements away, already used the explicit form (
NSEG16=INT(HT1PRD*2.0)) and produced no warning, so this simply makes the neighbours consistent.INT(ANINT(...))is used rather thanNINT(...)because it is the literal spelling of what the compiler already emits; the two are numerically equal, butNINTis a distinct intrinsic that can lower to a different instruction sequence.Two
DOloops terminated by an assignment rather thanEND DOorCONTINUE— a feature deleted in Fortran 2018 — converted to blockDO. Labels36and254existed only as loop terminators; label253, which sits on the second of those twoDOstatements, is a liveGOTOtarget (branched to from two places) and was preserved in columns 1–5.63 hard tab characters replaced with 6 spaces each. gfortran's fixed-form extension already advances a tab in columns 1–6 to column 7, so every effective column is unchanged. Two unused labels removed (
r10vol1.f:898,r10tap.f:201), each on a statement that generates no code.The two
unused_dummy_argumentwarnings inr10vol1.f(MTOPS,SPFLG) are left alone deliberately —R10VOL1's argument list is identical toR10VOL's, and dropping either would break that parity and any external caller's signature.Test evidence
The strongest available check is not a numerical comparison but a direct one: compile the file before and after and diff the generated assembly.
r10vol1.f,-O0and-O2.filedirective differs, naming the temp copies)r10tap.f,-O0and-O2r10vol1.fcvtss2sd/cvtsd2ss, 37powf, all unchanged)Compiled without
-gso that debug line tables — which do shift, since the block-DOconversion adds two lines tor10vol1.f— do not enter the comparison.A byte-identical result is stronger than a numerical test suite for a change of this kind: it says the compiler produced the same machine code, which holds for every possible input rather than for the inputs a test happens to sample. The instruction census makes the point plainly — the
REAL*8declaration was generating no double-precision code at all.That evidence matters here for a second reason, disclosed below.
How to reproduce
With nothing but a stock gfortran:
One thing to flag:
r10vol1.fhas no callersIn this repository
R10VOL1is never called — by anything.git log -S "R10VOL1("shows it never has been, andnmover all objects reportsr10vol1_,r10_hts_,r10gdib_,r10tc_andr10mlen_as defined and referenced nowhere.R10MLENis not called even from within its own file, despite the comment at line 384 sayingR10VOL1calls it.The live Region 10 path is
volinit.f:468→R10VOL(r10vol.f:3) →R10VOLO(r10volo.f:4), andR10VOL1's argument list is byte-identical toR10VOL's.R10TC(r10vol1.f:980) is a near-duplicate ofR10TCO(r10volo.f:312), andR10_HTS(:505) mirrorsR10HTS(profile.f:1643). It looks like a superseded implementation that was never removed.That means no behavioural test can reach
r10vol1.f, which is exactly why the evidence above is compile-output rather than numerical output. It also raises a question we are not positioned to answer: isr10vol1.fstill wanted? You are far better placed to know whether FVS or a DLL consumer calls it — it is exported from the shared library and listed invollib.vfproj. If it is obsolete, deleting it is a worthwhile change.r10tap.fis genuinely live — reachable fromR10VOL/R10VOLO— and now compiles clean.Separate issue worth raising
In both files,
ISPis left uninitialized for any species outside042/242/098/351:There is no
ELSEand no default, and the species branch immediately below testsISPagainstIAC/IRC/ISS/IRA.r10tap.f:93-96is on the live path, and the Region 10 default tables include species264and000, neither of which matches. Not touched here, since any default we chose would change dispatch — happy to open a separate issue if useful.