Skip to content

Conversation

@itsjunetime
Copy link
Collaborator

The from_enum macro, before this PR, defined each macro to have variants of isize type. While this probably didn't really cause any issues in practice, this always made me a bit nervous. As far as I can tell, rust doesn't guarantee that enums with repr(rust) are automatically sized down to the smallest representation they could use (see the reference chapters on type layout and enums, maybe I missed something), so we might be passing all enums around as isizes right now (as opposed to their most optimal representation, which is a u8 in most cases).

So this PR, to fix that, allows specifying a repr for all enums created with from_enum and ensures that every constant passed in to be the value of a variant in that enum will actually fit within the given repr.

This pr also cleans up a few other small things, like using a fully-qualified path for $crate::error::Error and providing a way to convert from an enum into its c representation (e.g. c_int) losslessly (to ensure that we don't accidentally lose information when we add a new enum without checking what its largest value actually is).

@ginnyTheCat
Copy link
Collaborator

According to https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.discriminant.repr-rust the compiler is allowed to choose a smaller repr and does this in practice as well. Using a enum U2 { Zero, One, Two, Three } is quite common for example and the compiler not doing this optimization would "break" crates like smol_str by increasing their size and therefore removing the performance benefit they give.

Not sure about the repr part of this PR therefore. The rest looks good.

@itsjunetime
Copy link
Collaborator Author

Huh, yeah you're right. I kinda assumed that explicitly as isizeing the values of all the variants would make the enum store them as isizes, but that's not the case at all. Thanks for noticing that. I would argue that that section of the reference just allows rust to do that instead of guaranteeing it, but that doesn't really matter all that much.

I'll remove the from_enum changes.

@itsjunetime
Copy link
Collaborator Author

Ok, I've pushed some changes so that we don't have the unnecessary repr annotation anymore, but I've made sure we can still keep the guaranteed-lossless impl From<$name> for $c_type so that we don't have to have a bunch of questionable enum_instance as _ and can instead use enum_instance.into() (guaranteed lossless) or, even better, c_type::from(enum_instance) (guaranteed lossless AND the type is explicit)

@itsjunetime
Copy link
Collaborator Author

Ok @ginnyTheCat sorry to keep this PR open so long as i fiddle with types here and there

I've now made it so that the from_enum macro has a $c_type_from and a $c_type_to because of some weird issues with how bindgen translates c enums to c_ints on windows but c_uints on every other platform. This theoretically allows us to use simple TryFrom and Into calls for every mupdf-enum-like type between their rust counterparts, on all platforms.

If you'd be willing to just give it a once-over to make sure I'm not crazy about anything else, that would be so appreciated :)

@ginnyTheCat
Copy link
Collaborator

ginnyTheCat commented Oct 16, 2025

Looks good. Thanks for this work.

c enums to c_ints on windows but c_uints on every other platform

Yea, I think C leaves it open to the implementation whether enums are c_int or c_uint (or even c_char apparently).

@itsjunetime itsjunetime merged commit b6dea54 into messense:main Oct 16, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants