archlinux / alpm.rs

Rust bindings for libalpm
GNU General Public License v3.0
112 stars 21 forks source link

No more unnecessary string copies #5

Closed 2xsaiko closed 4 years ago

2xsaiko commented 4 years ago

Basically just replaces a bunch of Into<String> with AsRef<str>.

Morganamilo commented 4 years ago

CString::new requires an Into<Vec<u8>>. &str implements this making a vector and cloning itself into it.

This means that it would clone every time.

Taking String means the memory could be moved instead of copied.

Remember that c strings are null terminated, so anything we pass from rust needs to have that null byte pushed to it.

However, If a String has no extra capacity, pushing to the string may cause it to realloc, or even worse, reallocate, so I guss neither are idea. But I feel this way at least adds more control because it doesn't guarantee a copy.

Hell if you really wanted you could allocate the string with capacity of length + 1 to give yourself room for the byte. But that's probably a micro optimisation that's not needed.

Anyway, thanks, I get this is what you would typically do in rust, but for c strings it's a little different.

2xsaiko commented 4 years ago

Oh, whoops. I even looked at the constructor for CString and must have missed that it takes Into<Vec<u8>> and not &[u8] or anything like that. Well that's annoying.

I guess I can still PR the fixes for some of the stuff though that I put in here, like this.

Morganamilo commented 4 years ago

I never read the PR because I assumed it was just the Into/AsRef change. If you have other things you wanna change, make a new PR and I'll happily look at it.

2xsaiko commented 4 years ago

Yup, will do. I guess I'll close this for now, then.