refactor(caseconv): move the case-copy preconditions into the engine - #252
Merged
Merged
Conversation
internal/caseconv held the word-at-a-time case converter but pushed four
coupled, unchecked preconditions onto its callers: ToLowerCopy required
len(dst) == len(src), dst[:from] already populated, from a multiple of WordLen,
and from at or below the first byte the conversion changes -- the last one
because its overlapping tail store rewrites dst[n-WordLen:from) and is a no-op
only under that rule. Both public case packages re-derived the same prologue to
satisfy it.
Replace ToLowerCopy/ToUpperCopy with ToLowerFrom/ToUpperFrom, which take the
index the caller's own scan already found and own everything downstream of it:
the destination size, the aligned start offset and the verbatim prefix. Four
unchecked rules about someone else's buffer become one -- first must be the
scanner's own result -- and the alignment rule lives in the function whose tail
store depends on it. The old copy helpers had one caller each afterwards, so
they are folded in: two functions fewer, one frame fewer, and the tail's dead
sub-word branch goes with them now that the length precondition is explicit.
The tidier shape, collapsing both packages onto one all-lengths entry point,
does not survive measurement. benchstat, n=15, samples interleaved between the
revisions, stdlib control sub-benchmarks holding at ~:
delegating every length ToLower/http-get/fiber +46.87%
ToUpper/http-get-upper/fiber +42.76%
ToLower/empty/fiber +63.39%
ToLower/http-get/unsafe +65.10%
inline scan, delegate convert short input that changes +16.42%
64-byte unchanged +15.11%
"get" and "GET" are three bytes and are how fiber compares methods, and
strings.ToLower is the helper it calls most from this module. At those lengths
the cross-package call costs more than the conversion, and a body handling every
length is far past the inlining budget, so the call cannot be optimised away.
Keeping the sub-word scan inline and handing the found index across restores the
previous call count on every path; the final state is neutral to slightly
better, geomean -1.20% for strings and -0.73% for bytes over 84 sub-benchmarks.
So the duplication that remains is deliberate: four short table loops, where the
scan is the whole conversion. The measurement is recorded once, in the bytes
package, with its method and the benchmark cases that reproduce it; the strings
package points at it rather than keeping a second copy to drift against.
The engine had no test files of its own -- its sub-word tails and its from > 0
contract were reached from swar_test.go in the root package, two package
boundaries away. caseconv_test.go now drives the interface at 100% statement
coverage, and three mutants that previously survived the whole suite now fail:
write through src inside ToLowerFrom assertCaseParity
drop the alignment (convert from 0) Test_ConvertFrom_PrefixIsTakenOnTrust
drop swar.MatchRangeMask's high-bit clear Test_CaseConv_Lengths
Each needed a specific shape of assertion. Expectations are computed before the
call and src compared against an untouched copy after it, because an illegal
write through src is itself a case-fold and an expectation read from src
afterwards would match the corruption rather than catch it -- and that read-only
promise is what lets strings.ToLower pass an unsafe view over immutable string
memory. The alignment is invisible to any in-contract equality oracle, since
there the skipped prefix is unchanged either way, so it is pinned with an index
above the true first change: the case the doc warns mis-converts. And the filler
carries no letters plus a byte >= 0x80 at every fourth offset, so the non-ASCII
passthrough is exercised on both sides of the change point.
Writing that third test corrected the code's own explanation: the tail store is
a no-op for two different reasons over two regions -- above the aligned start it
recomputes bytes the loops already wrote, and below it, it folds over the
verbatim prefix and matches only because every byte below first is unchanged.
The comment now says both, which is what makes first's upper bound load-bearing
rather than advisory.
What remains in the root package tests the public case-folding surface, so
swar_test.go becomes casefold_test.go: it never tested package swar.
Also in this code: FirstLowerIndex's doc comment was stranded above
firstSetLane32, documenting the wrong function; the word width was spelled three
ways inside one function; and the sub-word loops used range-over-int although
their body mutates the loop variable, which range discards -- correct only while
the body returns.
Public API of the bytes and strings packages is byte-identical: signatures diff
clean against master, which matters because fiber calls into them at 48 sites.
Suite green under -race -shuffle=on, go vet, gofumpt and golangci-lint clean.
No README rows change: the arm64 catalog stays valid because this is
performance-neutral, and amd64 measurements belong in the separate block this
repository keeps for them (README.md:621-623).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K7Z8sz85kpAwQKxoeFBv7i
gaby
requested review from
ReneWerner87,
efectn and
sixcolors
and removed request for
a team
September 14, 2026 04:07
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #252 +/- ##
==========================================
- Coverage 93.65% 93.61% -0.05%
==========================================
Files 38 38
Lines 2442 2426 -16
==========================================
- Hits 2287 2271 -16
Misses 135 135
Partials 20 20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b4fb9769d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
🧰 Additional context used📓 Path-based instructions (1)Do not move exported functions or packages to `internal/` merely because they have no non-test in-module callers; this library intentionally exposes helpers for downstream modules.📄 CodeRabbit inference engine (AGENTS.md) Files:
📝 WalkthroughWalkthroughThe change adds allocate-and-return case conversion kernels, removes copy-into-destination helpers, updates bytes and strings callers, and adds focused randomized, boundary, prefix, and fuzz tests. ChangesCase conversion
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant BytesOrStrings as bytes or strings converter
participant Caseconv as internal/caseconv
participant Result as converted result
BytesOrStrings->>Caseconv: find first byte needing conversion
BytesOrStrings->>Caseconv: call ToLowerFrom or ToUpperFrom
Caseconv->>Result: allocate and return converted copy
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No current merge-blocking behavior regression was identified in the case-conversion migration. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. I’m a rabbit with bytes in my paws Comment |
ReneWerner87
approved these changes
Sep 14, 2026
16 tasks
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
3 participants
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.
Description
internal/caseconvowns the word-at-a-time case converter, but it pushed four coupled, unchecked preconditions onto its callers.ToLowerCopy(dst, src, from)required thatlen(dst) == len(src), thatdst[:from]be already populated, thatfrombe a multiple ofWordLen, and thatfrombe at or below the first byte the conversion changes. The last rule exists only because the overlapping tail store rewritesdst[n-WordLen:from)— a no-op under that rule, a silent mis-conversion outside it. Both public case packages re-derived the same prologue to satisfy all four, and the engine had no way to reject a caller that got one wrong.ToLowerCopy/ToUpperCopyare replaced byToLowerFrom/ToUpperFrom, which take the index the caller's scan already found and own everything downstream of it: the destination size, the aligned start offset, and the verbatim prefix. Four unchecked rules about someone else's buffer become one —firstmust be the scanner's own result — and the alignment rule now lives in the function whose tail store depends on it. The old copy helpers had exactly one caller each afterwards, so they are folded in: two functions fewer, one frame fewer, and the tail's dead sub-word branch goes with them now that the length precondition is explicit.The exported surface of
internal/caseconvstays at nine symbols; the coupled preconditions go from four to one. The public API ofbytesandstringsis byte-identical — signatures diff clean againstmaster, which matters because Fiber calls into them at 48 sites.Fixes # (issue)
No linked issue — this came out of a module-boundary review of this repository.
Changes introduced
Benchmarks: the change is performance-neutral, and reaching that took measurement rather than taste. The tidier shape — collapsing both public packages onto one all-lengths entry point in
caseconv— does not survive benchmarking. Withbenchstat,n=15, samples interleaved between compiled test binaries for the two revisions, and the stdlib control sub-benchmarks holding at~:ToLower/http-get/fiberToUpper/http-get-upper/fiberToLower/empty/fiberToLower/http-get/unsafe"get"and"GET"are three bytes and are how Fiber compares methods;strings.ToLoweris the helper it calls most from this module. At those lengths the cross-package call costs more than the table lookups, and a body handling every length is far past the inlining budget, so the call cannot be optimised away. Keeping the sub-word scan inline and handing the found index across restores the previous call count on every path. Final state, same method: geomean −1.20% forstrings, −0.73% forbytesacross 84 sub-benchmarks, no sub-benchmark materially regressed. The remaining duplication (four short table loops, where the scan is the conversion) is therefore deliberate, and the measurement is recorded once — inbytes/case.go, with its method and the benchmark cases that reproduce it — withstrings/case.gopointing at it rather than keeping a second copy to drift against.Measured on
linux/amd64, Go 1.26. NoREADME.mdrows change: thedarwin/arm64catalog stays valid because the change is performance-neutral, and amd64 measurements belong in the separate block this repository keeps for them (README.md:621-623).Documentation Update: doc comments only, which is where the contracts under discussion live.
ToLowerFromnow states its one precondition and why a largerfirstmis-converts;ToUpperFromcross-references it; the read-only promise aboutsrcis stated explicitly, because that is what permitsstrings.ToLowerto pass an unsafe view over immutable string memory. Two pre-existing defects fixed along the way:FirstLowerIndex's doc comment was stranded abovefirstSetLane32, documenting the wrong function, and the word width was spelled three different ways inside one function (nowWordLen/4*WordLenthroughout).Changelog/What's New: not applicable — this repository has no changelog file; release notes come from commit subjects.
Migration Guide: not needed. The public API is unchanged; the renamed functions are in
internal/, so no importable symbol moved.API Alignment with Express: not applicable — no Express-facing surface is involved.
API Longevity: the change is in the direction of longevity. The interface a caller has to hold in their head shrinks from four coupled rules about a buffer they own to one rule about an index they already computed, and the invariant that makes the tail store sound now sits inside the function that performs it, so it cannot be violated from outside the package.
Examples: not applicable for an internal engine refactor.
Tests
internal/caseconvhad no test files of its own — its sub-word tails and itsfrom > 0contract were only reached fromswar_test.goin the root package, two package boundaries away. The newinternal/caseconv/caseconv_test.godrives the interface directly at 100.0% statement coverage, and three mutants that previously survived the entire suite (including 2M fuzz executions) now fail:srcinsideToLowerFromassertCaseParity0)Test_ConvertFrom_PrefixIsTakenOnTrustswar.MatchRangeMask's high-bit clearTest_CaseConv_LengthsEach needed a specific shape of assertion. Expectations are computed before the call and
srccompared against an untouched copy after it — an illegal write throughsrcis itself a case-fold, so an expectation read fromsrcafterwards would match the corruption instead of catching it. The alignment is invisible to any in-contract equality oracle (there the skipped prefix is unchanged either way), so it is pinned with an index above the true first change: exactly the case the doc warns mis-converts. And the filler carries no letters plus a byte>= 0x80at every fourth offset, so the non-ASCII passthrough is exercised on both sides of the change point.Writing that third test corrected the code's own explanation: the tail store is a no-op for two different reasons over two regions — above the aligned start it recomputes bytes the loops already wrote, and below it, it folds over the verbatim prefix and matches only because every byte below
firstis unchanged. The comment now says both, which is what makesfirst's upper bound load-bearing rather than advisory.swar_test.gois renamed tocasefold_test.go: what remains in the root package tests the public case-folding surface, and it never tested packageswar.Type of change
The template has no "refactor" option; this is closest. There is no behaviour change and no performance claim to make — the benchmarks above exist to show the refactor costs nothing, not to claim a win.
Checklist
Followed the inspiration of the Express.js framework for new functionalities— not applicable; no new functionality and no Express-facing API.firstcontract, both no-op clauses of the tail store, and the performance measurement with its method.Updated the documentation in the— not applicable; this repository has no/docs/directory/docs/. Doc comments are updated, as described above.make test(-race -shuffle=on) green,go vetclean,gofumptclean,golangci-lintreports 0 issues.Commit formatting
Single commit, conventional-commit style, matching the prevailing convention in this repository's history:
refactor(caseconv): move the case-copy preconditions into the engine🤖 Generated with Claude Code
https://claude.ai/code/session_01K7Z8sz85kpAwQKxoeFBv7i
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests