Make AsciiSet values instead of refs - non-breaking version #976
Conversation
percent_encoding/src/lib.rs
Outdated
| @@ -114,10 +114,10 @@ pub fn percent_encode_byte(byte: u8) -> &'static str { | |||
| /// assert_eq!(percent_encode(b"foo bar?", NON_ALPHANUMERIC).to_string(), "foo%20bar%3F"); | |||
| /// ``` | |||
| #[inline] | |||
| pub fn percent_encode<'a>(input: &'a [u8], ascii_set: &'static AsciiSet) -> PercentEncode<'a> { | |||
| pub fn percent_encode<'a, T: Into<AsciiSet>>(input: &'a [u8], ascii_set: T) -> PercentEncode<'a> { | |||
There was a problem hiding this comment.
ah, I see
thought: I'm not a huge fan of making things generic just for ref-or-value support, callers should handle that. So I'd rather this just be a non-generic function taking an AsciiSet. I do feel that the current use of refs is suboptimal. @valenting what do you think of doing a breaking release for percent-encoding? I don't think it's used directly by many, and I think it's an internal dep for rust-url so can be freely updated (though please check that)
There was a problem hiding this comment.
I agree it's worth considering breaking this, as the fix is often going to be simple and compiler error driven.
Edit: Reverse deps: https://crates.io/crates/percent-encoding/reverse_dependencies (900 crates) :/
There was a problem hiding this comment.
Hm. Still fine to do a breaking 3.x bump, and I don't think rust-url or any of the other libraries in this crate export it as a public dependency, so it can be bumped without changing everything.
But yes, not ideal.
There was a problem hiding this comment.
A breaking version of this change is in #978
|
ugh - I realized that this includes code which is already merged. Will rebase and push |
|
@joshka Would you be down to landing all the other changes except for moving things away from Ref as a separate PR? Those seem rather straightforward to land and it would be nice to get them. |
Definitely |
joshka
force-pushed
the
jm/asciiset-value
branch
from
215feb9
to
23bd642
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #976 +/- ##
=======================================
Coverage ? 82.23%
=======================================
Files ? 22
Lines ? 3563
Branches ? 0
=======================================
Hits ? 2930
Misses ? 633
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
Rebased on the refactoring PR and removed the already landed commits |
joshka
force-pushed
the
jm/asciiset-value
branch
from
540ecd1
to
d025bd8
Compare
Refs take 8 bytes, whereas the values are only 16 bytes, so there is not a huge benefit to using references rather than values. PercentEncoding is changed to store the AsciiSet as a value, and the functions that take AsciiSet now take Into<AsciiSet> instead of &'static AsciiSet. This allows existing code to continue to work without modification. The AsciiSet consts (CONTROLS and NON_ALPHANUMERIC) are also changed to be values, which is a breaking change, but will only affect code that attempts to dereference them. Discussion about the rationale for this is change is at <servo#970 (comment)>
joshka
force-pushed
the
jm/asciiset-value
branch
from
d025bd8
to
888596e
Compare
Refs take 8 bytes, whereas the values are only 16 bytes, so there is not a huge benefit to using references rather than values. PercentEncoding is changed to store the AsciiSet as a value, and the functions that previously accepted a reference now accept a value. This is a breaking change for users who were passing a reference to AsciiSet to the functions in the public API. The AsciiSet consts (CONTROLS, NON_ALPHANUMERIC, etc.) are also changed to be values. This is an alternative to the non-breaking change in <servo#976> Discussion about the rationale for this is change is at <servo#970 (comment)>
joshka
changed the title
Refs take 8 bytes, whereas the values are only 16 bytes, so there is not
a huge benefit to using references rather than values. PercentEncoding
is changed to store the AsciiSet as a value, and the functions that take
AsciiSet now take Into instead of &'static AsciiSet. This
allows existing code to continue to work without modification. The
AsciiSet consts (CONTROLS and NON_ALPHANUMERIC) are also changed to be
values, which is a breaking change, but will only affect code that
attempts to dereference them.
Discussion about the rationale for this is change is at
#970 (comment)