implement AddAssign for String #34890

Merged
merged 3 commits into from Jul 21, 2016

Conversation

Projects
None yet
6 participants
@oconnor663
Contributor

oconnor663 commented Jul 17, 2016

Currently String implements Add but not AddAssign. This PR fills in that gap.

I played around with having AddAssign (and Add and push_str) take AsRef<str> instead of &str, but it looks like that breaks arguments that implement Deref<Target=str> and not AsRef<str>. Comments in libcore/convert.rs make it sound like we could fix this with a blanket impl eventually. Does anyone know what's blocking that?

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 17, 2016

Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Jul 17, 2016

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@apasel422 apasel422 added the T-libs label Jul 18, 2016

src/libcollections/string.rs
@@ -1559,6 +1559,14 @@ impl<'a> Add<&'a str> for String {
}
}
+#[stable(feature = "rust1", since = "1.11.0")]

This comment has been minimized.

@apasel422

apasel422 Jul 18, 2016

Member

This line caused a make tidy error:

different `since` than before
@apasel422

apasel422 Jul 18, 2016

Member

This line caused a make tidy error:

different `since` than before

This comment has been minimized.

@oconnor663

oconnor663 Jul 18, 2016

Contributor

Ah, what's the right thing to do with these stable annotations in a PR?

@oconnor663

oconnor663 Jul 18, 2016

Contributor

Ah, what's the right thing to do with these stable annotations in a PR?

This comment has been minimized.

@alexcrichton

alexcrichton Jul 18, 2016

Member

Ah it's ok to pick an arbitrary feature name here, and the since I believe at this point should be 1.12.0

@alexcrichton

alexcrichton Jul 18, 2016

Member

Ah it's ok to pick an arbitrary feature name here, and the since I believe at this point should be 1.12.0

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 18, 2016

Member

cc @rust-lang/libs, seems reasonable to me given that Add is stabilized

Member

alexcrichton commented Jul 18, 2016

cc @rust-lang/libs, seems reasonable to me given that Add is stabilized

src/libcollections/string.rs
@@ -1559,6 +1559,14 @@ impl<'a> Add<&'a str> for String {
}
}
+#[stable(feature = "rust1", since = "1.12.0")]

This comment has been minimized.

@apasel422

apasel422 Jul 18, 2016

Member

I think you need to pick a different (arbitrary) feature here.

@apasel422

apasel422 Jul 18, 2016

Member

I think you need to pick a different (arbitrary) feature here.

This comment has been minimized.

@oconnor663

oconnor663 Jul 18, 2016

Contributor

Thanks, now I understand what you guys were saying above.

@oconnor663

oconnor663 Jul 18, 2016

Contributor

Thanks, now I understand what you guys were saying above.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 18, 2016

Contributor

sgtm

Contributor

brson commented Jul 18, 2016

sgtm

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 18, 2016

Contributor

Libs team discussed and said, 'yeap'.

@bors r+

Contributor

brson commented Jul 18, 2016

Libs team discussed and said, 'yeap'.

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 18, 2016

Contributor

📌 Commit 9b81306 has been approved by brson

Contributor

bors commented Jul 18, 2016

📌 Commit 9b81306 has been approved by brson

@brson brson added the relnotes label Jul 18, 2016

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 20, 2016

Contributor

⌛️ Testing commit 9b81306 with merge d87d602...

Contributor

bors commented Jul 20, 2016

⌛️ Testing commit 9b81306 with merge d87d602...

bors added a commit that referenced this pull request Jul 20, 2016

Auto merge of #34890 - oconnor663:addassign, r=brson
implement AddAssign for String

Currently `String` implements `Add` but not `AddAssign`. This PR fills in that gap.

I played around with having `AddAssign` (and `Add` and `push_str`) take `AsRef<str>` instead of `&str`, but it looks like that breaks arguments that implement `Deref<Target=str>` and not `AsRef<str>`. Comments in [`libcore/convert.rs`](https://github.com/rust-lang/rust/blob/master/src/libcore/convert.rs#L207-L213) make it sound like we could fix this with a blanket impl eventually. Does anyone know what's blocking that?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 20, 2016

Contributor

💔 Test failed - auto-win-gnu-32-opt-rustbuild

Contributor

bors commented Jul 20, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 20, 2016

Member

@bors: retry

On Tue, Jul 19, 2016 at 9:31 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1827


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34890 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95D-QEQ7QiYhovywnkS7nyFidwEeEks5qXaSjgaJpZM4JOS2O
.

Member

alexcrichton commented Jul 20, 2016

@bors: retry

On Tue, Jul 19, 2016 at 9:31 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1827


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34890 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95D-QEQ7QiYhovywnkS7nyFidwEeEks5qXaSjgaJpZM4JOS2O
.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Jul 20, 2016

@bors: rollup

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 21, 2016

Contributor

⌛️ Testing commit 9b81306 with merge 4c1a383...

Contributor

bors commented Jul 21, 2016

⌛️ Testing commit 9b81306 with merge 4c1a383...

bors added a commit that referenced this pull request Jul 21, 2016

Auto merge of #34890 - oconnor663:addassign, r=brson
implement AddAssign for String

Currently `String` implements `Add` but not `AddAssign`. This PR fills in that gap.

I played around with having `AddAssign` (and `Add` and `push_str`) take `AsRef<str>` instead of `&str`, but it looks like that breaks arguments that implement `Deref<Target=str>` and not `AsRef<str>`. Comments in [`libcore/convert.rs`](https://github.com/rust-lang/rust/blob/master/src/libcore/convert.rs#L207-L213) make it sound like we could fix this with a blanket impl eventually. Does anyone know what's blocking that?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 21, 2016

Contributor

💔 Test failed - auto-linux-64-cargotest

Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-linux-64-cargotest

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 21, 2016

Member

@bors: retry

On Wed, Jul 20, 2016 at 6:17 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cargotest
https://buildbot.rust-lang.org/builders/auto-linux-64-cargotest/builds/1199


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34890 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95JXSq-1RxtlpblIRUCUu-M_wT6xOks5qXsikgaJpZM4JOS2O
.

Member

alexcrichton commented Jul 21, 2016

@bors: retry

On Wed, Jul 20, 2016 at 6:17 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cargotest
https://buildbot.rust-lang.org/builders/auto-linux-64-cargotest/builds/1199


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34890 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95JXSq-1RxtlpblIRUCUu-M_wT6xOks5qXsikgaJpZM4JOS2O
.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 21, 2016

Rollup merge of #34890 - oconnor663:addassign, r=brson
implement AddAssign for String

Currently `String` implements `Add` but not `AddAssign`. This PR fills in that gap.

I played around with having `AddAssign` (and `Add` and `push_str`) take `AsRef<str>` instead of `&str`, but it looks like that breaks arguments that implement `Deref<Target=str>` and not `AsRef<str>`. Comments in [`libcore/convert.rs`](https://github.com/rust-lang/rust/blob/master/src/libcore/convert.rs#L207-L213) make it sound like we could fix this with a blanket impl eventually. Does anyone know what's blocking that?

@bors bors merged commit 9b81306 into rust-lang:master Jul 21, 2016

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment