Remove by-value From conversions between strings
#1286
Conversation
Rationale: there is no "move into" or "reuse buffer" optimization possible when converting between String, GString, StringName and NodePath. However, consuming the source string suggests that there is such a benefit. Implementing From only on references makes this clearer and leaves the original string intact for further use.
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1286 |
| @@ -1334,7 +1334,7 @@ impl<T: ArrayElement> GodotType for Array<T> { | |||
| // Typed arrays use type hint. | |||
| PropertyHintInfo { | |||
| hint: crate::global::PropertyHint::ARRAY_TYPE, | |||
| hint_string: GString::from(element_godot_type_name::<T>()), | |||
| hint_string: GString::from(&element_godot_type_name::<T>()), | |||
There was a problem hiding this comment.
Aside: I wonder if element_godot_type_name and thus godot_type_name could/should return &str instead. Probably someone thought about this before and the answer is "no" 😅
| } | ||
| } | ||
|
|
||
| impl From<&String> for NodePath { | ||
| fn from(s: &String) -> Self { | ||
| GString::from(s).into() | ||
| Self::from(&GString::from(s)) |
There was a problem hiding this comment.
I don't like the double conversion, but I couldn't find a way in the GDExtension API to construct a NodePath directly from UTF-8 bytes. Maybe add a comment to clarify while you're here?
| fn from(string_name: StringName) -> Self { | ||
| Self::from(GString::from(string_name)) | ||
| fn from(s: &StringName) -> Self { | ||
| Self::from(&GString::from(s)) |
It's also idiomatic. And
Could do, but I think it's overkill. |
There are two main problems I have with
I think there is a benefit of named conversions besides this: you can discover convertible types by just typing Either way, I'm wondering about the migration path here. Breaking code without deprecation isn't great, and |
The required changes are simple and fairly mechanical, as seen in this PR – mostly just adding |
Closes #1259.
Rationale: there is no "move into" or "reuse buffer" optimization possible when converting between String, GString, StringName and NodePath. However, consuming the source string suggests that there is such a benefit.
Implementing
Fromonly on references makes this clearer and leaves the original string intact for further use.This change has the potential to break quite a bit of code and make a few occurrences less nice to use,
GString::from(&s)instead ofs.into(). While I personally do like the explicitness, it's still rather wordy.One way would be to introduce named methods
to_gstring,to_string_name,to_node_pathover time. Might need a new traitStringConv, as inherent methods couldn't supportString, possibly one of the most common sources.At that point, it begs the question whether we still need
Frombetween strings at all, also in light of our API design. The strings are more or less the only place where we still useFromin such an extent... 🤔