Skip to content
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

NOT FOR MERGE: Initial rustification of hb_direction_t. #101

Closed
wants to merge 1 commit into from

Conversation

@waywardmonkeys
Copy link
Collaborator

waywardmonkeys commented Jan 16, 2018

Thinking about #90 ...

This is one way that we could use enums and is the result of using --rustified-enum with bindgen.

But looking at this, there's this option as well, which I think is better:

#[repr(u32)]
#[repr(C)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub enum Direction {
    /// Initial, unset direction.
    Invalid = 0,
    /// Text is set horizontally from left to right.
    LTR = 4,
    /// Text is set horizontally from right to left.
    RTL = 5,
    /// Text is set vertically from top to bottom.
    TTB = 6,
    /// Text is set vertically from bottom to top.
    BTT = 7,
}
pub type hb_direction_t = Direction;

Since harfbuzz has a stable API, we don't have to always run bindgen and it might be nicer to have an API that is what we'd actually want in Rust in terms of naming. (And then fixing up other issues in a harfbuzz crate.)

Thoughts?


This change is Reviewable

@waywardmonkeys
Copy link
Collaborator Author

waywardmonkeys commented Jan 16, 2018

Worth noting that by allowing ourselves to diverge from the bindgen output, we also can improve the documentation.

@jdm
Copy link
Member

jdm commented Jan 16, 2018

I'm in favour of the snippet you describe.

@waywardmonkeys
Copy link
Collaborator Author

waywardmonkeys commented Jan 16, 2018

@jdm Okay .. once #99 and #102 land, I can update this PR to include all enums and in the style pasted above.

@waywardmonkeys
Copy link
Collaborator Author

waywardmonkeys commented Jan 18, 2018

@mbrubeck Any thoughts on this?

@waywardmonkeys waywardmonkeys force-pushed the waywardmonkeys:rustify-enums branch 3 times, most recently from 3fc556c to fa52f25 Jan 19, 2018
@waywardmonkeys waywardmonkeys force-pushed the waywardmonkeys:rustify-enums branch from fa52f25 to f093dfa Jan 23, 2018
@waywardmonkeys
Copy link
Collaborator Author

waywardmonkeys commented Jan 23, 2018

@jdm I've updated the enums that aren't flags now ... what do you think? Still no word from @mbrubeck ...

@mbrubeck
Copy link
Contributor

mbrubeck commented Jan 23, 2018

I'm worried that this is unsafe. For example, if a newer version of Harfbuzz adds new values to the hb_script_t enum, and I use it with a Rust program that calls hb_buffer_get_script from this version of the harfbuzz-sys crate, then it could return an invalid value, which would result in undefined behavior.

@jdm
Copy link
Member

jdm commented Jan 23, 2018

True. I guess there's a reason to separate higher-level bindings that use types like enum and provide fallible conversions between the lower-level representation.

@waywardmonkeys
Copy link
Collaborator Author

waywardmonkeys commented Jan 31, 2018

Okay. I've been thinking about this some and I'm going to close this one and propose something else.

@waywardmonkeys waywardmonkeys deleted the waywardmonkeys:rustify-enums branch Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.