semantics: explicit mutating APIs, safe replacements#186
semantics: explicit mutating APIs, safe replacements#186ReneWerner87 merged 10 commits intomasterfrom
Conversation
Summary of ChangesHello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the utility library by clarifying the semantics of string and byte slice case conversion. It introduces explicit mutating functions for performance-critical scenarios while ensuring that the default conversion functions are non-mutating. This change improves API predictability, reduces the risk of unintended side effects, and streamlines internal code, contributing to a more robust and maintainable codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #186 +/- ##
==========================================
- Coverage 84.52% 84.15% -0.37%
==========================================
Files 12 14 +2
Lines 1053 1155 +102
==========================================
+ Hits 890 972 +82
- Misses 136 152 +16
- Partials 27 31 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces in-file ASCII case-conversion implementations with new internal packages/tables and unsafe helpers, adds dedicated bytes/strings case routines (safe and unsafe variants), updates top-level wrappers to delegate, and refactors tests/benchmarks and README benchmark formatting. No public API signatures changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@strings_test.go`:
- Around line 165-173: The "fiber/mut" sub-benchmark inside Benchmark_ToUpper is
missing b.ResetTimer(), so the setup allocation of template/work is being
measured; add a call to b.ResetTimer() after preparing template and work (before
the for loop) in the same sub-benchmark that calls
ToUpperMut(UnsafeString(work)) so timing matches Benchmark_ToLower's "fiber/mut"
case; locate the block using the identifiers Benchmark_ToUpper, "fiber/mut",
ToUpperMut, and UnsafeString and insert b.ResetTimer() immediately before the
benchmarking loop.
🧹 Nitpick comments (6)
xml_test.go (1)
91-93:b.ResetTimer()is redundant when usingb.Loop()."The b.ResetTimer at the loop's start and b.StopTimer at its end are integrated into testing.B.Loop", so the explicit
b.ResetTimer()on line 92 (and similarly on lines 114 and 131) can be removed. Not harmful, but unnecessary noise.🧹 Suggested cleanup (applies to all three benchmarks in this file)
b.ReportAllocs() - b.ResetTimer() for b.Loop() {time_test.go (1)
79-81:bb.ResetTimer()is redundant withbb.Loop().Same as in
xml_test.go—b.Loop()automatically resets the timer on its first call. The explicitbb.ResetTimer()calls on lines 80, 88, 97, and 107 can be removed.byteseq_test.go (1)
370-377:b.ResetTimer()is redundant withb.Loop().Lines 372 and 382 (and similarly 400, 410 in
Benchmark_TrimSpaceBytes) have explicitb.ResetTimer()calls that are unnecessary when usingb.Loop().convert_test.go (1)
289-299: LGTM with minor nit:b.ResetTimer()on line 293 is redundant withb.Loop().The migration is correct. The
Benchmark_ToString_concurrencyfunction properly retainspb.Next()forRunParallel, which is the correct pattern.bytes_test.go (2)
88-111: Inconsistent benchmark loop style: mixedb.Loop()andfor n := 0; n < b.N; n++.Within
Benchmark_ToLowerBytes, the "fiber/mut" sub-benchmark (line 99) usesfor b.Loop()while "fiber" (line 90) and "default" (line 107) usefor n := 0; n < b.N; n++. The same inconsistency exists inBenchmark_ToUpperBytes(lines 126, 135, 143), where all three use the manual loop. Since the PR objective is to modernize benchmark loops, consider usingb.Loop()consistently across all sub-benchmarks, or keeping the manual style consistently — mixing them makes the benchmarks harder to compare.
81-82:tc := tcis unnecessary in Go 1.22+.Since Go 1.22, loop variables are scoped per iteration, so this re-capture is no longer needed. Same applies to
Benchmark_ToUpperBytesat line 118.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.mdbytes.gobytes_test.gobyteseq_test.gocommon_test.goconvert_test.goformat_test.gohttp_test.goips_test.goparse_test.gostrings.gostrings_test.gotime_test.goxml_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
common_test.go (1)
common.go (2)
GenerateSecureToken(72-91)UUIDv4(54-60)
ips_test.go (1)
ips.go (2)
IsIPv4(15-45)IsIPv6(49-143)
time_test.go (1)
time.go (1)
Timestamp(18-20)
bytes_test.go (2)
bytes.go (4)
ToLowerBytes(8-33)ToUpperBytes(67-92)ToLowerBytesMut(37-64)ToUpperBytesMut(96-123)strings.go (2)
ToLower(8-36)ToUpper(47-76)
strings.go (2)
bytes.go (2)
ToLowerBytesMut(37-64)ToUpperBytesMut(96-123)convert.go (1)
UnsafeBytes(26-29)
byteseq_test.go (1)
byteseq.go (4)
EqualFold(8-54)TrimRight(83-89)TrimLeft(57-63)Trim(66-80)
convert_test.go (1)
convert.go (2)
UnsafeBytes(26-29)UnsafeString(18-23)
http_test.go (1)
http.go (2)
ParseVendorSpecificContentType(53-95)StatusMessage(104-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Build (1.24.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (14)
ips_test.go (1)
110-141: LGTM — clean migration tob.Loop().All four benchmark sub-functions correctly use the new
for b.Loop()idiom. No redundant timer calls, no other issues.parse_test.go (1)
55-55: LGTM — consistentb.Loop()migration across all parse benchmarks.All benchmark sub-functions are correctly updated.
b.ReportAllocs()is appropriately retained alongsideb.Loop().Also applies to: 64-64, 73-73, 155-155, 164-164, 173-173
common_test.go (1)
298-337: LGTM — cleanb.Loop()adoption across all common benchmarks.No redundant timer management calls, consistent pattern throughout.
byteseq_test.go (1)
62-63: LGTM —b.Loop()migration for EqualFold and Trim benchmarks.All loop changes are correct and consistent.
Also applies to: 69-70, 81-82, 88-89
format_test.go (1)
155-156: LGTM — consistentb.Loop()adoption across all format benchmarks.All changes are mechanical and correct. No redundant timer calls.
Also applies to: 161-162, 184-185, 190-191
strings.go (2)
38-44: LGTM — well-documented unsafe contract.The implementation is clean: delegates to
ToLowerBytesMut(UnsafeBytes(b))and the unsafe precondition is clearly stated in the doc comment.
78-84: LGTM — mirrorsToLowerMutsymmetrically.http_test.go (1)
57-75: LGTM — benchmark modernized tob.Loop()with expanded MIME coverage.bytes_test.go (1)
40-78: Good coverage of both non-mutation and in-place mutation contracts.The tests correctly verify that
ToLowerBytes/ToUpperBytesdon't modify the input and thatToLowerBytesMut/ToUpperBytesMutshare the same backing array.strings_test.go (2)
112-132: Good tests for the unsafe mutating variants.Tests correctly create a mutable buffer, wrap it with
UnsafeString, call the mutating function, and verify both the returned string and the backing buffer are modified.
46-58: LGTM — compact, representative benchmark case set.Good coverage of empty, small (GET), mixed-case header, and large strings in different case configurations.
bytes.go (2)
7-33: LGTM — clean non-mutating implementation with unconditional copy.Always allocating a new
dstslice is a sound design choice: it guarantees the input is never modified and avoids subtle aliasing bugs, even at the cost of an allocation when the input is already lowercase.
35-64: LGTM — well-structured in-place mutating variants.The unrolled loop pattern is consistent between
ToLowerBytesMutandToUpperBytesMut, and correctly handles the remainder after the 4-byte chunks.Also applies to: 94-123
README.md (1)
22-348: LGTM — benchmark table updated to reflect the new mutating variants and refreshed numbers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Code Review
This pull request does a great job of improving API clarity by separating mutating and non-mutating functions for case conversion. The introduction of ...Mut functions makes the in-place behavior explicit, which will help prevent accidental mutations. The associated test and benchmark updates are thorough. I've left a couple of minor suggestions regarding benchmark consistency.
There was a problem hiding this comment.
Pull request overview
This PR clarifies case-conversion semantics by introducing explicit in-place (mutating) APIs alongside safe non-mutating defaults, and updates benchmarks/tests to reflect the new split and Go 1.24 benchmark-loop idioms.
Changes:
- Added explicit mutating string and byte-slice ASCII case-conversion APIs (
ToLowerMut/ToUpperMut,ToLowerBytesMut/ToUpperBytesMut) and adjusted tests accordingly. - Updated
ToLowerBytes/ToUpperBytesto be non-mutating (safe default) implementations. - Refactored/updated benchmarks across the repo to use
b.Loop()and added new benchmark variants for mutating paths; refreshed benchmark numbers in the README.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bytes.go | Implements non-mutating byte-slice case conversion and adds explicit in-place variants. |
| strings.go | Adds explicit mutating string case-conversion APIs that operate on mutable-backed strings. |
| bytes_test.go | Adds mutation/non-mutation tests and benchmarks for byte-slice case conversion (incl. mutating variant). |
| strings_test.go | Adds tests/benchmarks for mutating string APIs and simplifies benchmark case set. |
| format_test.go | Updates benchmarks to use b.Loop() for iteration. |
| parse_test.go | Updates benchmarks to use b.Loop() for iteration. |
| time_test.go | Updates benchmarks to use b.Loop() for iteration. |
| xml_test.go | Updates benchmarks to use b.Loop() for iteration. |
| http_test.go | Updates benchmarks to use b.Loop() for iteration. |
| ips_test.go | Updates benchmarks to use b.Loop() for iteration. |
| convert_test.go | Updates benchmarks to use b.Loop() for iteration. |
| common_test.go | Updates benchmarks to use b.Loop() for iteration. |
| byteseq_test.go | Updates benchmarks to use b.Loop() for iteration. |
| README.md | Updates the published benchmark output to reflect new benchmark structure/results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/format_benchmarks.go`:
- Line 49: In the second pass switch that inspects the variable trimmed and
toggles inBlock, replace the stale check trimmed == "```go" with trimmed ==
"```text" so it matches the first-pass detection of text fenced benchmark
blocks; update the conditional that sets inBlock (and any corresponding case
branch) to use "```text" instead of "```go".
🧹 Nitpick comments (1)
bytes_test.go (1)
87-92: Minor inconsistency in benchmark assertions:bytes.Equalvsrequire.Equal.
Benchmark_ToLowerBytesusesrequire.True(b, bytes.Equal(want, res))(line 92) whileBenchmark_ToUpperBytesusesrequire.Equal(b, want, res)(line 126). Both work, butrequire.Equalgives better failure messages. Consider aligning onrequire.Equalfor both.Also applies to: 121-126
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdbytes_test.goconvert_test.goformat_test.goscripts/format_benchmarks.gostrings_test.goxml_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- strings_test.go
- format_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
bytes_test.go (1)
bytes.go (4)
ToLowerBytes(8-45)ToUpperBytes(79-116)ToLowerBytesMut(49-76)ToUpperBytesMut(120-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (1.24.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (3)
xml_test.go (1)
93-93: LGTM — consistent loop variable rename.Cosmetic rename from
itonaligns with the benchmark convention used across the PR.Also applies to: 115-115, 132-132
convert_test.go (1)
294-294: LGTM — consistent loop variable rename.Also applies to: 375-375
bytes_test.go (1)
40-78: Well-structured mutation and no-mutation tests.Good coverage: verifying that
ToLowerBytes/ToUpperBytesdon't mutate the input, and that theMutvariants share the same backing array. The use ofrequire.Samefor pointer identity is a nice touch.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request successfully introduces a clear distinction between mutating and non-mutating case conversion APIs, enhancing both safety and performance. The implementation uses efficient ASCII lookup tables and loop unrolling. I've noted a few opportunities for further optimization in the 'no-change' path for byte slices, potential issues with aliasing in the safe byte slice APIs, and some inconsistencies in loop constructs that affect maintainability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… in-place conversion
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bytes.go`:
- Around line 11-17: Update the deprecation comment for ToLowerBytes (and
similarly ToUpperBytes) to recommend the safe, non-mutating replacement
bytes.ToLower from "github.com/gofiber/utils/v2/bytes" as the primary
alternative, and mention bytes.UnsafeToLower only for callers that explicitly
need in-place mutation for performance; keep the note that this wrapper
preserves backward compatibility by mutating the provided slice and reference
the specific functions ToLowerBytes, ToUpperBytes, bytes.ToLower, and
bytes.UnsafeToLower so readers can find the alternatives easily.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bytes.go
🧰 Additional context used
🧬 Code graph analysis (1)
bytes.go (1)
bytes/case.go (2)
UnsafeToLower(81-104)UnsafeToUpper(108-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Compare
🔇 Additional comments (2)
bytes.go (2)
7-9: LGTM!Good use of an explicit import alias (
casebytes) to avoid shadowing the standard librarybytespackage.
27-32: No changes — looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…proved readability
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Why I changed this
The previous setup made it too easy to mix up copy-based vs in-place behavior.
For string/byte case conversion, that distinction is important for correctness and for performance expectations.
What changed
Outcome
Summary by CodeRabbit
New Features
Tests
Documentation
Refactor