perf(builtin): expand Hasher::combine_{string,bytes} to accept views#3460
Open
bobzhang wants to merge 1 commit into
Open
perf(builtin): expand Hasher::combine_{string,bytes} to accept views#3460bobzhang wants to merge 1 commit into
bobzhang wants to merge 1 commit into
Conversation
Both methods only read their argument, so requiring ownership is gratuitous. Now they accept `StringView` / `BytesView` respectively — existing `String` / `Bytes` values still coerce on argument passing (expanding mbti change, no caller breaks). With `combine_string` accepting `StringView`, the `Hash for StringView` impl collapses from a hand-rolled loop to a one-line delegation — the two paths were doing the same thing anyway. `Hash for BytesView` is left alone: its loop uses a different packing scheme than `combine_bytes`, so delegating would change observable hash values for every existing `Bytes` consumer. Align the internal private helper `endian32` to `BytesView` too so the chain works end-to-end.
Collaborator
Coverage Report for CI Build 3914Coverage decreased (-0.001%) to 94.869%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This was referenced Apr 24, 2026
bobzhang
added a commit
that referenced
this pull request
Apr 24, 2026
`Buffer::write_bytes(value : Bytes)` and `Buffer::write_bytesview(value : BytesView)` were doing the same thing with different parameter types. Expand the former to `BytesView` and make it delegate — existing `Bytes` callers keep working via coercion, and the implementation is now one line instead of two parallel blit loops kept in sync. `write_bytesview` stays for now (callers may have reached for the explicit name); in a future cleanup it can be deprecated once downstream migrates to just using `write_bytes`. Continues the view-safe expansion theme (#3459, #3460, #3461).
bobzhang
added a commit
that referenced
this pull request
Apr 24, 2026
Both `BigInt::from_octets` and `ChaCha8::new` only read their byte argument — neither stores it nor mutates through it — so there's no reason to require ownership. Callers who already have a `BytesView` (for example a slice into a larger buffer, or a view handed back from an encoder) can now pass it directly without materializing a fresh `Bytes`. This is an expanding mbti change: every existing `Bytes` caller still compiles because `Bytes` coerces to `BytesView` at argument passing. Continues the "view-safe expansion" theme from #3459 / #3460.
1 task
bobzhang
added a commit
that referenced
this pull request
Apr 24, 2026
`fail`, `assert_true`, `assert_false`, and `@test.fail` all only ever
read their `msg` argument — it's either formatted into a `Failure` error
string via `"\{loc} FAILED: \{msg}"` or substituted as-is in an
interpolation. No reason to require ownership.
Expand the `msg` parameter of each to `StringView`. Every existing
caller keeps compiling — `String` values coerce to `StringView` at
argument passing, as do literal strings.
Continues the view-safe expansion theme (#3459, #3460, #3461, #3462).
bobzhang
added a commit
that referenced
this pull request
Apr 25, 2026
`fail`, `assert_true`, `assert_false`, and `@test.fail` all only ever
read their `msg` argument — it's either formatted into a `Failure` error
string via `"\{loc} FAILED: \{msg}"` or substituted as-is in an
interpolation. No reason to require ownership.
Expand the `msg` parameter of each to `StringView`. Every existing
caller keeps compiling — `String` values coerce to `StringView` at
argument passing, as do literal strings.
Continues the view-safe expansion theme (#3459, #3460, #3461, #3462).
bobzhang
added a commit
that referenced
this pull request
Apr 25, 2026
`Buffer::write_bytes(value : Bytes)` and `Buffer::write_bytesview(value : BytesView)` were doing the same thing with different parameter types. Expand the former to `BytesView` and make it delegate — existing `Bytes` callers keep working via coercion, and the implementation is now one line instead of two parallel blit loops kept in sync. `write_bytesview` stays for now (callers may have reached for the explicit name); in a future cleanup it can be deprecated once downstream migrates to just using `write_bytes`. Continues the view-safe expansion theme (#3459, #3460, #3461).
bobzhang
added a commit
that referenced
this pull request
Apr 25, 2026
`fail`, `assert_true`, `assert_false`, and `@test.fail` all only ever
read their `msg` argument — it's either formatted into a `Failure` error
string via `"\{loc} FAILED: \{msg}"` or substituted as-is in an
interpolation. No reason to require ownership.
Expand the `msg` parameter of each to `StringView`. Every existing
caller keeps compiling — `String` values coerce to `StringView` at
argument passing, as do literal strings.
Continues the view-safe expansion theme (#3459, #3460, #3461, #3462).
bobzhang
added a commit
that referenced
this pull request
Apr 25, 2026
`Buffer::write_bytes(value : Bytes)` and `Buffer::write_bytesview(value : BytesView)` were doing the same thing with different parameter types. Expand the former to `BytesView` and make it delegate — existing `Bytes` callers keep working via coercion, and the implementation is now one line instead of two parallel blit loops kept in sync. `write_bytesview` stays for now (callers may have reached for the explicit name); in a future cleanup it can be deprecated once downstream migrates to just using `write_bytes`. Continues the view-safe expansion theme (#3459, #3460, #3461).
bobzhang
added a commit
that referenced
this pull request
Apr 25, 2026
`Buffer::write_bytes(value : Bytes)` and `Buffer::write_bytesview(value : BytesView)` were doing the same thing with different parameter types. Expand the former to `BytesView` and make it delegate — existing `Bytes` callers keep working via coercion, and the implementation is now one line instead of two parallel blit loops kept in sync. `write_bytesview` stays for now (callers may have reached for the explicit name); in a future cleanup it can be deprecated once downstream migrates to just using `write_bytes`. Continues the view-safe expansion theme (#3459, #3460, #3461).
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
2 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.
Summary
Follow-on to #3459. The same argument — `StringView` / `BytesView` are views over immutable data, so substituting them for the owned types at read-only API boundaries is always safe — applies to the `Hasher` helpers:
Internal private helper `endian32` is similarly expanded so the `combine_bytes` chain compiles.
Expanding mbti change: every existing caller keeps working (`String` / `Bytes` values and literals coerce to their view types at argument passing).
Cleanup dividend
With `combine_string` now accepting `StringView`, the `Hash for StringView` impl collapses from a 9-line manual loop to a one-liner. The two were doing the same thing — iterate UTF-16 code units and call `combine_uint` on each — just in different code.
`Hash for BytesView` is not simplified the same way: its 4-byte-packing loop uses `combine_uint` / `combine_byte`, while `combine_bytes` uses the lower-level `consume4` / `consume1`. Delegating would change observable hash values for every existing `Bytes` consumer — not acceptable. Left as-is.
Test plan
🤖 Generated with Claude Code