Skip to content
This repository was archived by the owner on Mar 4, 2024. It is now read-only.

glib: Ergonomics of 'Char' seem questionalble #491

Closed
ids1024 opened this issue Apr 26, 2021 · 9 comments · Fixed by #494
Closed

glib: Ergonomics of 'Char' seem questionalble #491

ids1024 opened this issue Apr 26, 2021 · 9 comments · Fixed by #494
Milestone

Comments

@ids1024
Copy link
Contributor

ids1024 commented Apr 26, 2021

The docs suggest this is meant to be more ergonomic that taking libc:c_char. But is it an improvement over just accepting u8? Rust has a b'' syntax.

For the set_list_separator example in the documentation:

key_file.set_list_separator(glib::Char::new(';').unwrap());
key_file.set_list_separator(b';');
key_file.set_list_separator(';' as u8);

Apparently u8 doesn't implement TryFrom<char> (rust-lang/rfcs#2854), which would be useful for panicking behavior similar to glib::Char::new(...).unwrap() when out of range. But on the whole, accepting a u8 seems more ergonomic for most uses.

@ids1024
Copy link
Contributor Author

ids1024 commented Apr 26, 2021

For reference, libc::c_char seems to be i8 or u8 depending on the platform. I believe under the C standard a char has to be at least 8 bits, and sizeof(char) has to be 1, but it could technically be more than 8 bits. I doubt Rust or gtk are really usable on some bizarre hardware with bytes that aren't 8 bits though.

So it should be fine to use u8, which is the most natural type in Rust (given that b'' is interpreted as a u8).

@sdroege sdroege added this to the 0.14.0 milestone Apr 27, 2021
@sdroege sdroege added the glib label Apr 27, 2021
@sdroege
Copy link
Member

sdroege commented Apr 27, 2021

I agree, and also if we keep Char we should remove its constructor and make it a TryFrom impl instead.

@bilelmoussaoui @GuillaumeGomez ?

@GuillaumeGomez
Copy link
Member

I'm fine with replacing Char constructor.

@sdroege
Copy link
Member

sdroege commented Apr 28, 2021

@GuillaumeGomez What advantage over using u8 do you see that justifies the existence of the type? Not using it and directly using u8 seems easier in all cases I can think of.

@GuillaumeGomez
Copy link
Member

Char is more explicit than u8. That's the only reason. We could use libc::c_char too but I find it less "nice".

@ids1024
Copy link
Contributor Author

ids1024 commented Apr 28, 2021

libc::c_char seems like an awkward type to use (except for ffi) since it could be a u8 or i8, and the b'' byte char syntax can only be used with u8.

So I guess glib::Char(b';') currently works on some platforms but fails to compile on others. So even if a newtype pattern like this is preferred, perhaps it should contain a u8 rather than c_char.

@GuillaumeGomez
Copy link
Member

We can simply add a method on Char: fn new(c: u8) -> Self to make it coherent between platforms.

@sdroege
Copy link
Member

sdroege commented Apr 28, 2021

So a TryFrom<char> for Char and a From<u8> for Char. Ok with me

@GuillaumeGomez
Copy link
Member

👍

ids1024 added a commit to ids1024/gtk-rs that referenced this issue Apr 28, 2021
See gtk-rs#491

This leaves `fn new()` unchanged. Don't know if it would be better to
change or remove that.
@sdroege sdroege linked a pull request Apr 28, 2021 that will close this issue
ids1024 added a commit to ids1024/gtk-rs that referenced this issue Apr 28, 2021
See gtk-rs#491

This leaves `fn new()` unchanged. Don't know if it would be better to
change or remove that.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants