Skip to content

Add impl AddAssign<char> for Cow<'_, str> #66215

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.

Sign up for GitHub

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

Closed
wants to merge 15 commits into from
Closed

Add impl AddAssign<char> for Cow<'_, str> #66215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
76a3b55
Added AddAssign<char> implementation for String
Ruster-a11y Nov 8, 2019
ed52372
Amended with the suggested syntax+improvements
Ruster-a11y Nov 8, 2019
342277f
Ammended further to accomodate `AddAssign<char>` for `String` and `Co…
Ruster-a11y Nov 8, 2019
9b55f6b
Added tests for the respective AddAssign/Add operations for `char` co…
Ruster-a11y Nov 9, 2019
cd2ad6d
Re-implemented so that Cow::Borrowed now gets allocated using the inh…
Ruster-a11y Nov 11, 2019
12a32e6
Replaced wrongly used s.len() with lhs.len()
Ruster-a11y Nov 11, 2019
543a0b6
Added String::reserve_exact() to reserve the optimal space.
Ruster-a11y Nov 11, 2019
ea5f056
Changed code logic to call RawVec::amortized_new_size() from prealloc…
Ruster-a11y Nov 11, 2019
7d87504
Allocate new string using inline amortized logic.
Ruster-a11y Nov 16, 2019
d62e867
Revert "Added tests for the respective AddAssign/Add operations for `…
Ruster-a11y Nov 20, 2019
f2ac395
Revert "Ammended further to accomodate `AddAssign<char>` for `String`…
Ruster-a11y Nov 20, 2019
0704d9b
Revert "Ammended further to accomodate `AddAssign<char>` for `String`…
Ruster-a11y Nov 20, 2019
5623044
Revert to the start of PR
Ruster-a11y Nov 20, 2019
5d2184b
impl Add<char> and AddAssign<char> for Cow<'_, str>
Ruster-a11y Nov 20, 2019
47d0179
Fixed unused variable error in tests\cow_str.rs
Ruster-a11y Nov 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/liballoc/borrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#![stable(feature = "rust1", since = "1.0.0")]

use core::cmp::Ordering;
use core::cmp::{Ordering, max};
use core::hash::{Hash, Hasher};
use core::ops::{Add, AddAssign, Deref};

Expand Down Expand Up @@ -444,6 +444,17 @@ impl<'a> Add<Cow<'a, str>> for Cow<'a, str> {
}
}

#[stable(feature = "cow_str_add_char", since = "1.41.0")]
impl<'a> Add<char> for Cow<'a, str> {
type Output = Cow<'a, str>;

#[inline]
fn add(mut self, rhs: char) -> Self::Output {
self += rhs;
self
}
}

#[stable(feature = "cow_add", since = "1.14.0")]
impl<'a> AddAssign<&'a str> for Cow<'a, str> {
fn add_assign(&mut self, rhs: &'a str) {
Expand Down Expand Up @@ -479,3 +490,18 @@ impl<'a> AddAssign<Cow<'a, str>> for Cow<'a, str> {
}
}
}

#[stable(feature = "cow_str_add_assign_char", since = "1.41.0")]
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//Attempt amortized memory allocation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let new_optimal_size = max(base_capacity * 2, base_capacity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let mut s = String::with_capacity(new_optimal_size);
s.push_str(lhs);
*self = Cow::Owned(s);
}
self.to_mut().push(rhs);
}
}
29 changes: 29 additions & 0 deletions src/liballoc/tests/cow_str.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,20 @@ fn check_cow_add_str() {
}
}

#[test]
fn check_cow_add_char() {
let test_char = '👋';

let borrowed = Cow::Borrowed("Hello, World! ");

let owned: Cow<'_, str> = Cow::Owned(String::from("Hi, World! "));

assert_eq!("Hello, World! 👋", borrowed.clone() + test_char);

assert_eq!("Hi, World! 👋", owned.clone() + test_char);
}


#[test]
fn check_cow_add_assign_cow() {
let mut borrowed1 = Cow::Borrowed("Hello, ");
Expand Down Expand Up @@ -139,3 +153,18 @@ fn check_cow_clone_from() {
c1.clone_from(&c2);
assert!(c1.into_owned().capacity() >= 25);
}

#[test]
fn check_cow_add_assign_char() {
let test_char = '👋';

let mut borrowed = Cow::Borrowed("Hello, World! ");

let mut owned: Cow<'_, str> = Cow::Owned(String::from("Hi, World! "));

owned += test_char;
borrowed += test_char;

assert_eq!(format!("Hi, World! {}", test_char), owned);
assert_eq!(format!("Hello, World! {}", test_char), borrowed);
}