Repository navigation
fix(recovery): keep the charge trace's negative zero, and correct the skin-temp comment - #1446
Merged
Merged
Conversation
… skin-temp comment (#1437 follow-up) Two residuals from the rounding/formatting parity work in #1437 and #1440. #1437 aligned the half-tie rule but left the near-zero case divergent. Math.round returns a Long, which has no negative zero, so negating it BEFORE the division collapsed -0.0 to +0.0: for any term in (-0.005, 0) — a value just below baseline — Android printed `z=0.0` where Swift's .rounded() keeps -0.0 and prints `z=-0.0`. These values are interpolated straight into the trace, so the two platforms disagreed on the one line whose entire purpose is being byte-identical. Parenthesising the division negates the Double instead; verified on a JVM that every half-tie and ordinary value is unchanged (-0.125 → -0.13, 0.125 → 0.13) and only the near-zero negatives move. #1440's comment described a bug that does not exist: `%+.1f` renders -0.0 as "-0.0", not "-+0.0". The real defect it fixed was the hand-built sign it replaced — `-0.0 >= 0` is TRUE, so "+" was prepended to a value formatting as "-0.0", printing "+-0.0". The code was right; the explanation would have misled the next reader. Mirrored regression fixtures pin the near-zero term on both platforms. Android 4,066 tests / 0 failures; the Swift twin is a package test, so swift-packages CI executes it.
Re-review of my own fix: parenthesising the division closed only ONE of the two traps in this helper. `-0.0 < 0.0` is FALSE, so a sign COMPARISON cannot route an exact negative zero into the negating branch at all — it takes the else branch and comes back +0.0. And -0.0 is reachable here, not hypothetical: the skin-temp term is z = -|dev| / scale, so a deviation of exactly 0.0 (skin temp sitting on the personal baseline) produces -0.0, which Swift prints as z=-0.0. Rounding the magnitude and reapplying the sign with copySign handles both traps at once: the Long that has no negative zero, and the comparison that cannot see one. Verified on a JVM across -0.0, small negatives, half-ties in both directions and positives. Second mirrored vector added for the exact -0.0 case. Android 4,067 tests / 0 failures.
Third pass on my own comment. Correcting #1440's claim, I asserted that `%+.1f` cannot distinguish the two zeroes for the "+0.0" case. That is also false: C printf and Java's Formatter both render -0.0 as "-0.0" and 0.0 as "+0.0", checked directly. So the explicit zero branch is redundant on the formatters we have actually verified. The comment now says that, and gives the honest reason the branch stays: it pins the edge so the two zeroes cannot drift from the Kotlin twin on a Foundation whose formatter has not been checked. The bug the branch replaced is unchanged and correctly described - a sign built from `dev >= 0`, which is TRUE for -0.0, prepending "+" to a value already formatting as "-0.0".
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.
Two residuals left behind by the rounding/formatting parity work in @bhelm's #1437 and #1440.
Both were found by re-checking those merges rather than by a report.
1. The near-zero case was still divergent (#1437)
#1437 correctly aligned the half-tie rule, but
Math.roundreturns aLong, which has no negativezero — so negating it before the division collapsed
-0.0to+0.0:For any term in
(-0.005, 0)— a value sitting just below baseline — Android printedz=0.0whereSwift's
.rounded()keeps-0.0and printsz=-0.0. These go straight into the trace by stringinterpolation, so the two platforms disagreed on the one line whose entire purpose is being
byte-identical.
Verified on a JVM that moving the paren changes nothing else:
-0.125 → -0.13,-0.126 → -0.13,0.125 → 0.13all identical; only the near-zero negatives move.Severity is low — this is diagnostic text, not a score, and #1437's own test already pins that the
traced score equals the plain scorer's. But byte-identical trace lines are exactly what that helper
exists for.
2. The #1440 comment describes a bug that does not exist
It claims Foundation renders negative zero as
-+0.0. It does not —%+.1fof-0.0is-0.0.The real defect #1440 fixed was the hand-built sign it replaced:
-0.0 >= 0is true, so"+"wasprepended to a value that formatted as
"-0.0", printing+-0.0. The code was correct; only theexplanation was wrong, and it would have misled whoever touched it next. Rewritten to state the actual
mechanism and why both zeroes are handled explicitly.
Verification
--rerun-tasks, new test confirmed by name)Packages// Android, soswift-packagesCI executes the Swift twin — noapp-target Swift touched, so no app-build gate needed