Skip to content
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

String::truncate() and Vec::truncate() are inconsistent if new_len > current length #32717

Closed
Dr-Emann opened this issue Apr 4, 2016 · 4 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Dr-Emann
Copy link

Dr-Emann commented Apr 4, 2016

String::truncate specifies that it "panics if new_len > current length"
Vec::truncate specifies that "If len is greater than the vector's current length, this has no effect.

This seems very inconsistent. I would personally prefer Vec's semantics. I'm not sure if it's a breaking change to add/remove a documented panic, and I fear it's not okay, but it feels very inconsistent.

@Aatch
Copy link
Contributor

Aatch commented Apr 4, 2016

Looking at the source, I have a feeling the panic-on-greater-than isn't a deliberate decision. Since it simply asserts that the new length is a character boundary (and presumably "not in the string isn't a valid boundary"). I think adding a panic could be considered a breaking change, but removing one doesn't seem like it should be.

@Aatch Aatch added A-libs T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 4, 2016
@Stebalien
Copy link
Contributor

@Aatch getting rid of a panic is just as bad if not worse:

unsafe {
    s.truncate(10); // panics if s isn't long enough.
    s.as_mut_vec().as_mut_ptr() = b"1234567890";
}

This is obviously a stupid example but you get the point.

@brson
Copy link
Contributor

brson commented Apr 13, 2016

Ignoring the panics seems an ok solution to me.

@alexcrichton
Copy link
Member

This was discussed at libs triage and the decision was that this is indeed an inconsistency we'd like to fix and the window for fixing still seems open as the code this would break seems almost too niche to exist. I've submitted a PR for this.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 15, 2016
The `Vec::truncate` method does not panic if the length argument is greater than
the vector's current length, but `String::truncate` will indeed panic. This
semantic difference can be a bit jarring (e.g. rust-lang#32717), and after some
discussion the libs team concluded that although this can technically be a
breaking change it is almost undoubtedly not so in practice.

This commit changes the semantics of `String::truncate` to be a noop if
`new_len` is greater than the length of the current string.

Closes rust-lang#32717
bors added a commit that referenced this issue Apr 17, 2016
std: Change String::truncate to panic less

The `Vec::truncate` method does not panic if the length argument is greater than
the vector's current length, but `String::truncate` will indeed panic. This
semantic difference can be a bit jarring (e.g. #32717), and after some
discussion the libs team concluded that although this can technically be a
breaking change it is almost undoubtedly not so in practice.

This commit changes the semantics of `String::truncate` to be a noop if
`new_len` is greater than the length of the current string.

Closes #32717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants