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

mark str::string::String.trim.* functions as #[must_use]. #57106

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@matthiaskrgr
Copy link
Contributor

matthiaskrgr commented Dec 24, 2018

The functions return a reference to a new object and do not modify in-place
as the following code shows:

let s = String::from("   hello   ");
s.trim();
assert_eq!(s, "   hello   ");

The new reference should be bound to a variable as now indicated by #[must_use].

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 24, 2018

r? @cramertj

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

Show resolved Hide resolved src/libcore/str/mod.rs Outdated
@zackmdavis

This comment has been minimized.

Copy link
Member

zackmdavis commented Dec 24, 2018

Generalized discussion on where we want to do this is in #48926.

@matthiaskrgr matthiaskrgr force-pushed the matthiaskrgr:trim_must_use branch from eded81a to 01f76d3 Dec 25, 2018

@@ -3554,6 +3554,8 @@ impl str {
///
/// assert_eq!("Hello\tworld", s.trim());
/// ```
#[must_use = "this returns the trimmed string as a new allocation, \

This comment has been minimized.

@zackmdavis

zackmdavis Dec 25, 2018

Member

I'm pretty sure trim returns a slice into the existing string and therefore doesn't allocate? (Sorry my earlier comment wasn't clearer: I linked to the must_use reason on String.replace just as an example of the reason functionality being used. Consistent language style is desirable, but we don't want to reuse exact words when that would be inaccurate.)

Show resolved Hide resolved src/libcore/str/mod.rs Outdated
@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 26, 2018

r? @sfackler
(libs team member who was following the original issue)

@rust-highfive rust-highfive assigned sfackler and unassigned cramertj Dec 26, 2018

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 26, 2018

Yep, none of these return a new allocation, so the message here should be changed.

matthiaskrgr and others added some commits Dec 24, 2018

mark str::string::String.trim.* functions as #[must_use].
The functions return a reference to a new object and do not modify in-place
as the following code shows:
````
let s = String::from("   hello   ");
s.trim();
assert_eq!(s, "   hello   ");
````

The new reference should be bound to a variable as now indicated by #[must_use].
Update src/libcore/str/mod.rs, tweak must_use message
trimmed string is returned as a slice instead of a new allocation

Co-Authored-By: matthiaskrgr <matthias.krueger@famsik.de>

@matthiaskrgr matthiaskrgr force-pushed the matthiaskrgr:trim_must_use branch from aa74cfc to 74e9057 Dec 26, 2018

@kennytm kennytm added the relnotes label Dec 27, 2018

@Centril Centril added the T-libs label Jan 10, 2019

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 12, 2019

Ping from triage, @sfackler :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment