use String::new() instead of String::from(""), "".to_string(), "".to_owned() or "".into()

[matthiaskrgr]

No description provided.

[rust-highfive]

r? @varkor

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

[matthiaskrgr]

Motivation: rust-lang/rust-clippy#2972

[varkor]

Thanks!

@bors r+ rollup

[bors]

📌 Commit e97ccd89d8b2d5f5c9038a293f19a474358b3404 has been approved by varkor

[killercup]

Another option would be to make "".to_string() call String::new(). I have never looked at the code so I don't know what issues this may have.

But following the breadcrumbs…

rust/src/liballoc/string.rs

Lines 2160 to 2165 in 70c33bb

	impl ToString for str {
	#[inline]
	fn to_string(&self) -> String {
	String::from(self)
	}
	}

rust/src/liballoc/string.rs

Lines 2200 to 2204 in 70c33bb

	impl<'a> From<&'a str> for String {
	fn from(s: &'a str) -> String {
	s.to_owned()
	}
	}

rust/src/liballoc/str.rs

Lines 206 to 208 in 70c33bb

	fn to_owned(&self) -> String {
	unsafe { String::from_utf8_unchecked(self.as_bytes().to_owned()) }
	}

…it seems we could try to add a if bytes.is_empty() { return String::new() } here and see what happens:

rust/src/liballoc/string.rs

Lines 731 to 733 in 70c33bb

	pub unsafe fn from_utf8_unchecked(bytes: Vec<u8>) -> String {
	String { vec: bytes }
	}

[matthiaskrgr]

Mh yes, I also wondered if there was some way to have some sort of shortcut.
But i was not sure how expensive the .is_empty() check would be.

Is there some way to only emit the check if we know the input of the function is a constant and is guaranteed to be optimized out (by always taking the return String::new() codepath)?

[killercup]

Is there some way to only emit the check if we know the input of the function is a constant and is guaranteed to be optimized out (by always taking the return String::new() codepath)?

I think this would require dependent types, or all the functions I quoted to be const fn

Edit: Oh, and self.as_bytes().to_owned() is most likely calling clone on a Vec<u8> -- this could also be optimized to not allocate zero bytes (maybe it even already is)

Edit2: Well…

rust/src/liballoc/vec.rs

Lines 1686 to 1688 in a9d4967

	fn clone(&self) -> Vec<T> {
	<[T]>::to_vec(&**self)
	}

rust/src/liballoc/slice.rs

Lines 365 to 370 in a9d4967

	pub fn to_vec(&self) -> Vec<T>
	where T: Clone
	{
	// NB see hack module in this file
	hack::to_vec(self)
	}

rust/src/liballoc/slice.rs

Lines 164 to 171 in a9d4967

	pub fn to_vec<T>(s: &[T]) -> Vec<T>
	where T: Clone
	{
	let mut vector = Vec::with_capacity(s.len());
	vector.extend_from_slice(s);
	vector
	}
	}

[nagisa]

@matthiaskrgr The check for empty allocation is already done by "".as_bytes().to_owned() and already does avoid the allocation. The problem is that neither String::from() nor "".to_owned() are inlined. I suspect that’s entirely the cause of whatever the benchmark in the clippy issue measured.

[matthiaskrgr]

Oh, that's interesting!

When I tried setting a very high (9999....) or very low (1, 0) inline threshold on godbolt I still can't get String::from() to be inlined though. 😕
https://rust.godbolt.org/z/xDC-aW

[nagisa]

We’ll just have to add #[inline]s on the from and to_owned methods to make them available for inlining.

[bors]

☔ The latest upstream changes (presumably #53607) made this pull request unmergeable. Please resolve the merge conflicts.

[matthiaskrgr]

rebased
@varkor r?

[varkor]

@bors r+ rollup

[bors]

📌 Commit ede1f7d has been approved by varkor