-
Notifications
You must be signed in to change notification settings - Fork 11
Implement some missing std::string::String
methods
#28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Implement some missing std::string::String
methods
#28
Conversation
Hey! Thanks for the patch! After a couple of months I finally have a little moment to look at what's going on here D New methods are cool. If you would be so kind as to split it into smaller patches, one per new method perhaps, and start with the patches that retain the API and binary compatibility, I'm sure I'll find the time to check and then merge them. As it is, I see some hard decisions here, like how much the usefulness of a trait object and maybe the added link time overhead due to more generics compare to the usefulness of new methods and the strain on developers from having an extra version to be aware of, and I would prefer to simply avoid making those (but of course maybe @fitzgen will want to make those decisions himself). (I understant that the link time overhead here is likely neglible, but those add app and linking generics remains a bane of both C++ and Rust, I think. Libraries with simlper non-generic APIs are thus more welcome in my codebase; arguably generics also has a cognitive overhead for humans and not just compilers, cf. https://www.agilealliance.org/glossary/simple-design/ ?). |
Without And
I'll try to do that. |
This also fixes missing docs on 32-bit platforms and allows using this crate for platforms other than 32- and 64-bit.
3feaa5c
to
71b64e6
Compare
Strictly speaking, `truncate` not throwing panic with index >= length is a breaking change, but that was not very String-like anyway.
Can't the generics live on another trait? |
|
They can. But then to get it all together you must either do
Yeah, that's bad, but gonna change with one of the next commits. I'm just doing |
Right! But keep in mind that the larger the PR is the harder it is to review and accept D |
Just do it on the per-commit basis, no? |
Removed `Borrow<str>` requirement, so non-linear strings could implement this trait now. Removed trait-bound lifetime. Added `Sized` requirement to the trait itself, due to `with_capacity` being a required method (it can't be implemented for unsized structs).
Implement `TryFrom<&str>` instead of `From<&str>` for `InlineString`. Panics in `From` are bad. Really. Just read the trait docs: > Note: This trait must not fail. If the conversion can fail, use TryFrom. Plus, `TryFrom` allows for some nice safe capacity checks (the next commit will show all non-doc changes due to `TryFrom`).
Okay, I found unsoundness in the Plus, the test that I wrote and that must get this bug passes in debug mode due to |
Rly, mine error is better — it derives more traits and also implements `Display` and `From<Infallible>`.
That's all what was written yet. |
There are reasons for `as_bytes_mut` function being unsafe for strings, so this impl does not make sense, as it allows getting mutable slice without `unsafe` keyword.
The test struct for provided methods was implementing the wrong required method.
Closes #26
INLINE_STRING_CAPACITY
:No
drain
method; too much complicated code to write (even more complicated than it normally must be, because we are producing an iterator struct through trait there), whilechars
+remove_range
pair does exactly the same job. Probably I need to write that inremove_range
docs.Minimum Rust version is 1.42 due to
matches!
macro in tests; I dunno if 1.41 would be minimum if user doesn't compile tests (some traits forString
with 1.41+ orphan rules).Crate major (middle, cuz it's pre-1.) version must be updated.
StringExt
can no longer be a trait object.