gix-url: accept impl Into<&BStr> in parse#2606
gix-url: accept impl Into<&BStr> in parse#2606Sarthak Bhardwaj (SarthakB11) wants to merge 4 commits into
impl Into<&BStr> in parse#2606Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bd84ba539
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| /// We cannot and should never have to deal with UTF-16 encoded windows strings, so bytes input is acceptable. | ||
| /// For file-paths, we don't expect UTF8 encoding either. | ||
| pub fn parse(input: &BStr) -> Result<Url, parse::Error> { | ||
| pub fn parse<'a>(input: impl Into<&'a BStr>) -> Result<Url, parse::Error> { |
There was a problem hiding this comment.
.into() call sites compiling
With the parameter changed to a generic impl Into<&'a BStr>, existing callers that already wrote gix_url::parse("...".into()) no longer give the compiler a concrete target type for the inner .into(), so type inference fails with “cannot infer type” rather than selecting &BStr. This pattern still exists in this crate's tests (for example gix-url/tests/url/access.rs uses both string and byte-slice .into() inputs), and it is also a likely downstream call style from the previous API, so this signature change breaks those callers unless every call site removes .into() or the API keeps a concrete input type.
Useful? React with 👍 / 👎.
`BString::as_bstr` is private in some bstr feature configurations (notably the fuzz crate), which broke CI Build Fuzzers. `BStr::new()` is always public and serves the same purpose.
`parse(impl Into<&'a BStr>)` makes `data.into()` ambiguous; use `data.as_bstr()` via `bstr::ByteSlice` for an unambiguous `&BStr`.
`parse(impl Into<&'a BStr>)` accepts `&[u8]` directly via the `From<&[u8]> for &BStr` impl in bstr. The previous `.into()` was ambiguous after the signature widened.
|
Closing this for now. After pushing, CI surfaced ambiguous Two questions before I retry, if you have a preference:
Happy to take either direction. Closing this draft so the issue stays the discussion point. |
1 participant
Per #2468,
gix_url::parsenow takesimpl Into<&'a BStr>instead of&BStr. A&bstringborrow now suffices at the call site; existing&BStrcallers keep compiling via the reflexiveFrom<&BStr> for &BStrimpl inbstr. String literals also work directly throughFrom<&str> for &BStr.The signature widens to a borrowed
Into(Into<&'a BStr>) rather than the ownedInto<BString>shape the issue mentions, because callers in this codebase already hold borrowed bstrs and the borrowed form avoids an allocation. Thebstrre-export question raised in the same issue is out of scope here; happy to follow up.One internal
.as_ref()had to become.as_bstr()becauseBStringexposes severalAsRefimpls and the call was ambiguous after the signature change. Doc-test examples were left passing literals directly.Validation
cargo test -p gix-urlcargo check --workspacecargo doc -p gix-url(doc-test examples)Disclosure
This PR was drafted with Claude Code (Anthropic). I reviewed and verified the diff before submission.
Closes #2468