New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Char wrapper 2 #274

Merged
merged 6 commits into from Dec 17, 2017

Conversation

Projects
None yet
4 participants
@EPashkin
Member

EPashkin commented Dec 7, 2017

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 7, 2017

👍

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 7, 2017

@federicomenaquintero Please check this too. I removed From<Char> for i8, may be it inappropriate for your usage.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 7, 2017

Added forgoted FromGlib for UChar and test for it

@federicomenaquintero

This comment has been minimized.

Contributor

federicomenaquintero commented Dec 8, 2017

Thanks for the heads-up. I haven't worked on gnome-class for a little while, so I need to remind myself what I was doing :) I'll do that and see what's up with the changes in gir and glib.

src/char.rs Outdated
@@ -23,7 +27,7 @@
/// done in the `new` function; see its documentation for details.
///
/// The inner `i8` (which is equivalent to `libc::c_char` can be extracted with `.0`, or
/// by calling `i8::from(my_char)`.
/// by calling `my_char.to_glib()`.
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub struct Char(pub i8);

This comment has been minimized.

@sdroege

sdroege Dec 8, 2017

Member

Sorry for bringing up some C bullshit here, but char can be signed or unsigned depending on the platform. On PowerPC, char is unsigned for example (and gcc has a -fsigned-chars parameter for that very reason). And gchar in GLib is equivalent to char, so it might be signed or unsigned.

This comment has been minimized.

@sdroege

sdroege Dec 8, 2017

Member

Of course nothing in the C standard says that chars must be 8 bits, but let's ignore that for now. All platforms where GLib runs have 8 bit chars.

This comment has been minimized.

@EPashkin

EPashkin Dec 8, 2017

Member

Then maybe completely replace i8 with c_char ?
Then in PowerPC case we have Char(u8) and UChar(u8) (if it has this definition in libc)

This comment has been minimized.

@EPashkin

EPashkin Dec 8, 2017

Member

But then maybe problem with testing.

This comment has been minimized.

@sdroege

sdroege Dec 8, 2017

Member

Problem with testing what? But yes, that would make sense. It would at least mirror what happens in GLib then

This comment has been minimized.

@EPashkin

EPashkin Dec 8, 2017

Member

Currently in test just used 65i8 and 65u8, so "PowerPC" and other will need other tests.

This comment has been minimized.

@EPashkin

EPashkin Dec 8, 2017

Member

@GuillaumeGomez As I remember you prefer i32 over c_int or gint.
Can I make reverse here?

This comment has been minimized.

@EPashkin

EPashkin Dec 8, 2017

Member

Added replacement as fixup! commit, I do rebase after accepting this way.

fn from(c: UChar) -> char {
c.0 as char
}
}

This comment has been minimized.

@sdroege

sdroege Dec 8, 2017

Member

Should there be TryFrom impls for the other direction? Or rather, with TryFrom still being unstable, the same function implemented in impl Char / impl UChar?

This comment has been minimized.

@EPashkin

EPashkin Dec 8, 2017

Member

IMHO new will be replaced with TryFrom when it stabilized.
Or you meant something other?

This comment has been minimized.

@sdroege

sdroege Dec 8, 2017

Member

Indeed, nevermind then :)

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 17, 2017

@GuillaumeGomez and at this too, please

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 17, 2017

An easy one this time, great! Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 9340470 into gtk-rs:master Dec 17, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@EPashkin EPashkin deleted the EPashkin:char_wrapper_2 branch Dec 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment