Add impl AddAssign<char> for Cow<'_, str>
#66215
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
@Centril Please see. |
Centril
added
relnotes
|
r? @dtolnay cc @rust-lang/libs |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Centril
changed the title
impl AddAssign<char> for Cow<'_, str>
|
@Ruster-a11y I would also recommend amending the PR description to describe the use case & motivation for this change. |
|
@Centril On it. Thanks again! |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
…w<str>`. Added `impl Add<char> for String` to ascertain logical parity.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@dtolnay This needs further review. Edit: Respective test procedures have been added. |
…ncatenation to `String` and `Cow<str>`.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
dtolnay
added
S-waiting-on-author
|
@dtolnay Can you help me with an online ID of some sort to chat and resolve this with you in realtime? |
|
Hi there! Unfortunately, adding this impl is pretty tricky. I tried this once, too, but this lead to some breakage (of code in the compiler). See this issue: #51916 So I think we can't merge this PR before we discussed and "solved" the issue I linked. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Hello @LukasKalbertodt, I apologize for the late response. You're right, this feature would require downstream changes for the respective crates as per the compile-time error messages. Can you suggest anything to help kill this ambiguity that the compiler is unable to distinguish from? Surely, implementing a core solution would be better than changes across the previous code/logic of other crates. |
Unfortunately, no. I think the first step would be to find out why exactly this happens. And if this is indeed intended behavior (very likely) or not. I just asked on StackOverflow to maybe get an answer that way. I will update the issue if I completely understood the issue. Next (assuming this is intended behavior), we have to discuss what the balance between the benefits of I think this is roughly how we should proceed. I'm pretty sure there is no "quick fix" for now. |
|
Yeah, this is intended behavior. When the compiler looks for an impl, and ends up with only a single one that could possibly apply, it eagerly confirms that impl regardless of the current state of type inference, and uses the impls type parameters to drive type inference forward (by unifying them with any inference variables that apply). This means it now knows that the expected type is If there's more than one impl, the compiler can't select one of them yet and you end up with an unknown expected type, so no coercion can happen and it ends up leaving that type set to |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ping from triage: |
| if let Cow::Borrowed(lhs) = *self { | ||
| let base_capacity = lhs.len() + rhs.len_utf8(); | ||
| //Attempt amortized memory allocation | ||
| let new_optimal_size = max(base_capacity * 2, base_capacity); |
There was a problem hiding this comment.
This doesn't look right to me -- we know base_capacity >= 0 so max(base_capacity * 2, base_capacity) is always the same as base_capacity * 2. Has it been written this way to account for integer overflow? If so, that needs to be handled differently; the overflowing multiplication would not be guaranteed to wrap around to 0.
| fn add_assign(&mut self, rhs: char) { | ||
| if let Cow::Borrowed(lhs) = *self { | ||
| let base_capacity = lhs.len() + rhs.len_utf8(); | ||
| //Attempt amortized memory allocation |
There was a problem hiding this comment.
Is it possible to avoid duplicating this sizing heuristic from the existing amortized_new_size in raw_vec.rs? I thought you had used amortized_new_size successfully in an earlier revision.
If we make changes to the heuristic in the future, it should only be in one place.
| impl AddAssign<char> for Cow<'_, str> { | ||
| fn add_assign(&mut self, rhs: char) { | ||
| if let Cow::Borrowed(lhs) = *self { | ||
| let base_capacity = lhs.len() + rhs.len_utf8(); |
There was a problem hiding this comment.
This isn't quite right -- the amortized_new_size heuristic expects to receive the currently used capacity (how much is already used) and the requested extra capacity (how much is being added) as separate data points. In this AddAssign impl the currently used capacity is the length of the lhs Cow<str> and how much is being added is the size of the rhs char. Adding these together ahead of time would make the heuristic no longer applicable, or behave not as intended.
|
I think this PR can go on hold until we settle whether we can have Add<char> for String (#66504). I believe we wouldn't want this for Cow<str> if it can't exist on String. |
Dylan-DPC-zz
added
S-blocked
|
Marking this as blocked on #66504 |
|
Closing, as #66504 ended up not working out. Thanks anyway @Ruster-a11y! |
Centril
removed
the
relnotes
There are two obvious ways to append acharto aString:String.push(char)String += &(char.to_string())The second way is almost 3x slower for up to 1000 iterations and slows down much further when the iteration count increases.Since the second way might be preferred by some due to the legibility of+=operator -- anAddAssign<char>implementation forStringmakes sense without losing the speed benefits ofString.push(char)With this addition, the second way could be reconstructed simply as:String += charEdit:
The following operation is also possible now:new_string = old_string + charUpdate:
This PR has now been relegated to just an implementation for
impl AddAssign<char> for Cow<'_, str>. Further commits will ensure the PR code only effects/improves upon the said implementation.