-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
impl Add<char> and AddAssign<char> for String #66490
impl Add<char> and AddAssign<char> for String #66490
Conversation
Made previous code tidy friendly.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be kind if someone would test it on Linux as well.
That will be done automatically by CI. If you rebase your PR and solve the conflict in parser/expr.rs
(and force push), CI will run.
I commented a few things, but please note that I am not in the Rust team and some of these comments might be wrong :P
src/libsyntax/parse/parser/expr.rs
Outdated
// FIXME: Should just be `&symbol.as_str()` but can't as of now due to | ||
// Deref coercion. | ||
// Issue: https://github.com/rust-lang/rust/issues/51916 | ||
let s = String::from("0.") + &symbol.as_str().get_str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can write &*symbol.as_str()
to explicitly deref if I'm not mistaken. Then you don't need the get_str
method. And in that case, I would probably also remove the FIXME
comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this worked!
src/libsyntax_pos/symbol.rs
Outdated
/// | ||
/// FIXME: This has no meaning anymore since the addition of `impl Add<char> for String`. | ||
/// Due to the outstanding Deref coercion issue this Deref implementation gets ignored. | ||
/// Issue: https://github.com/rust-lang/rust/issues/51916 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the Deref
impl is still used for plenty of other stuff. The only thing that changed is that it's not automatically used anymore when using Add
or AddAssign
. I would just remove this comment, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/liballoc/string.rs
Outdated
/// Implements the `+` operator for concatenating two `String`s. | ||
/// | ||
/// This consumes the `String` on the left-hand side and re-uses its buffer (growing it if | ||
/// necessary). This is done to avoid allocating a new `String` and copying the entire contents on | ||
/// every operation, which would lead to `O(n^2)` running time when building an `n`-byte string by | ||
/// repeated concatenation. | ||
/// | ||
/// The string on the right-hand side is only borrowed; its contents are copied into the returned | ||
/// `String`. | ||
/// | ||
/// # Examples | ||
/// | ||
/// Concatenating two `String`s takes the first by value and borrows the second: | ||
/// | ||
/// ``` | ||
/// let a = String::from("hello"); | ||
/// let b = String::from(" world"); | ||
/// let c = a + &b; | ||
/// // `a` is moved and can no longer be used here. | ||
/// ``` | ||
/// | ||
/// If you want to keep using the first `String`, you can clone it and append to the clone instead: | ||
/// | ||
/// ``` | ||
/// let a = String::from("hello"); | ||
/// let b = String::from(" world"); | ||
/// let c = a.clone() + &b; | ||
/// // `a` is still valid here. | ||
/// ``` | ||
/// | ||
/// Concatenating `&str` slices can be done by converting the first to a `String`: | ||
/// | ||
/// ``` | ||
/// let a = "hello"; | ||
/// let b = " world"; | ||
/// let c = a.to_string() + b; | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole comment is just a copy from the Add<&str>
impl, right? I would probably:
- Add the information about the "problem" we are facing to the old comment for
Add<&str>
, i.e.: explain that we also had to add strange impls likeAdd<&String>
in order to avoid breakage. - Then add a short comment to all "strange" impls (
Add<&String>
,Add<&&String>
) referring to the main documentation onAdd<&str>
.
That way we don't have to have duplicated comments/documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've added a new commit fixing the comments. Please see if what I've revised them with makes sense.
src/liballoc/string.rs
Outdated
/// Implements the `+=` operator for appending to a `String`. | ||
/// | ||
/// This has the same behavior as the [`push_str`][String::push_str] method. | ||
#[stable(feature = "stringaddassign_string", since = "1.41.0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be fine to use the feature name extra_string_add_impls
or something like that everywhere. If we wanna keep the names you chose, I'd think that they should be proper snake case: string_add_assign_string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please check the new commit.
@LukasKalbertodt I don't truly understand the cause for this. What specific git commands should I use to fix it? |
This just means that on the I assume your
Then you should be able to:
Funnily, git can actually merge everything correctly it seems. |
ec2b1c6
to
c5e19ca
Compare
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 |
c5e19ca
to
ec2b1c6
Compare
This file was moved to a different directory so this isn't needed anymore.
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 |
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 |
@LukasKalbertodt I attempted to rebase my local git with the latest commit on master (0f0c640). somehow it's including various submodule changes even though I never made or staged those changes. What do you suggest? A new PR? |
Closing this and attempting with another PR to fix unforeseen rebase issues. |
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 |
This is the new working code for the functionalities mentioned in the title. This also means the previous pull request ( #66215) can now be relegated to just as the implementation for
AddAssign<char> for Cow<'_, str>
.This makes the following operations possible:
These further code modifications were needed due to the already open issue #51916. Since that issue is beyond the scope for this PR and feature-set, this commit only attempts at getting around that issue by further adding implementations for:
impl Add<&&str> for String
impl Add<&String> for String
impl Add<&&String> for String
impl AddAssign<&String> for String
Without these
impl
additions, the rust compiler complains about lacking the respective implementations forString
operations sinceimpl Add<char> for String
adds a layer of ambiguity for the compiler.@LukasKalbertodt, thanks for your suggestions! (#66215 (comment))
Note:
Build only tested on latest Windows 10. Would be kind if someone would test it on Linux as well.