Implement From<&[char]> for GString
#862
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-862 |
| @@ -69,12 +69,14 @@ fn string_chars() { | |||
| // Empty tests regression from #228: Null pointer passed to slice::from_raw_parts(). | |||
| let string = GString::new(); | |||
| assert_eq!(string.chars(), &[]); | |||
| assert_eq!(string, GString::from(&[][..])); | |||
There was a problem hiding this comment.
&[][..] is quite weird 🤔 please the expected variable separately and annotate with &[char] type.
There was a problem hiding this comment.
How about [].as_slice()?
There was a problem hiding this comment.
I would still add &[char] so it's clear that this conversion is being tested. Type erasure and compact expressions can be nice sometimes, but tests should primarily be readable and easy to understand. Any indirection to figure out that we're looking at a char slice (not just a slice) is an unnecessary obstacle.
And once you have that, [].as_slice() involves arrays without a very good reason, so you could just have &[] 😉
There was a problem hiding this comment.
To be fair; doesn't &[] involve arrays, too? 🤔
There was a problem hiding this comment.
You're right, that was a bit imprecise. What I meant is that &[1, 2, 3] is very common syntax, but [1, 2, 3].as_slice() makes readers think "what is this?"; same for empty ones 🙂
Also, sorry to be nitpicky, but I wrote "variable separately" above. You did an as cast instead.
assert_eq!(string, GString::from(&[] as &[char]));Please change this to
let empty: &[char] = &[];
assert_eq!(string, GString::from(empty));Again because people may wonder "why cast a type into itself" and lints may actually flag this in the future.
| let ctor = interface_fn!(string_new_with_utf32_chars_and_len); | ||
| ctor( | ||
| string_ptr, | ||
| chars.as_ptr() as *const char32_t, |
| @@ -225,6 +225,21 @@ impl From<&str> for GString { | |||
| } | |||
| } | |||
|
|
|||
| impl From<&[char]> for GString { | |||
| fn from(chars: &[char]) -> Self { | |||
| unsafe { | |||
There was a problem hiding this comment.
Since this isn't completely obvious, you should maybe add a // SAFETY line saying something about UTF-32.
|
|
||
| let string = String::from("some_string"); | ||
| let string_chars: Vec<char> = string.chars().collect(); | ||
| let gstring = GString::from(string); | ||
|
|
||
| assert_eq!(string_chars, gstring.chars().to_vec()); | ||
| assert_eq!(gstring, GString::from(&string_chars[..])); |
There was a problem hiding this comment.
Also here, a separate variable with an expressive name. The fact that you operate on &[char] slices here is very well-hidden, it needn't be.
Bromeon
changed the title
From<&[char]> for GString
|
|
||
| let string = String::from("some_string"); | ||
| let string_chars: Vec<char> = string.chars().collect(); | ||
| let gstring = GString::from(string); | ||
|
|
||
| assert_eq!(string_chars, gstring.chars().to_vec()); | ||
| assert_eq!(gstring, GString::from(string_chars.as_slice())); |
There was a problem hiding this comment.
In this case I'd say it's fine because Vec<char> is just explicitly declared above, and Vec::as_slice() is sufficiently clear 👍
Des-Nerger
force-pushed
the
master
branch
3 times, most recently
from
8b6974b
to
f8edeca
Compare
No description provided.