georust
Allow creating a Proj from an owned String
CString::new takes any Into<Vec<u8>>, which includes String. This should make it possible to avoid reallocating when passing in an owned String with sufficient capacity.
The test failures were caused by some doctests calling
Proj::new_known_crs(&from, &to, None)
where from is &str, so &from is &&str. Perhaps there is more code out there that relies on this working.
I think the TryFrom implementation extension doesn't cause breakage, since that never accepted &&str.
I think the existing type signature is going to be more intuitive for the caller. If I'm understanding your motivation, having to type in a & before your owned string does not seem like a big enough hurdle to justify this change.
Unless I'm misunderstanding, I don't think we should merge this.
Sorry, I missed your initial comment that this is about avoiding re-allocation inside of CString. But my conclusion is the same - it seems unlikely that gains here will be worth having a more opaque/confusing input type.
Are you experiencing this part of the code as a bottleneck?
I just find it a bit odd for Proj to ask for a reference if in the end it wants to own its input. (See C-CALLER-CONTROL.)
Having had another look, I don't think this is performance-critical for us, so I don't mind if you don't think it's worth it.
The type could also be Into<String>, which is clearer and more type-safe, but less flexible.
FWIW, in gdal we take &str because:
- if you can measure the performance difference, you're almost certainly doing something wrong
-
S: Into<Vec<u8>>is not a type I'd want our users to see, especially given that most of them will be coming from other languages, like Python - pushing the NUL byte will usually cause a reallocation anyway
I appreciate the attempt, but I think on balance this is not worthwhile, so closing.