Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use exact types for Py_BuildValue.#7853
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
2d70522 to74ab0a6CompareStupid questions cause I wanna wrap my head around this... What's the reference you're using to get specific types? And how come the new specific types don't have to be the same size as (and sometimes bigger than) the old types? |
I get the types from theFreeType API. IIRC, C ABI promotes all scalar values into the largest compatible type when passed to vararg functions like |
| @@ -18,8 +18,8 @@ extern "C" { | |||
| /* | |||
| By definition, FT_FIXED as 2 16bit values stored in a single long. | |||
| */ | |||
| #define FIXED_MAJOR(val) (long)((val & 0xffff000) >> 16) | |||
| #define FIXED_MINOR(val) (long)(val & 0xffff) | |||
| #define FIXED_MAJOR(val) (unsigned short)((val & 0xffff000) >> 16) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't that supposed to be a signed long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not exactly, but there appears to maybe be a deeper existing bug here. I'm not sure if I'm just reading it wrong, but will need some time to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What areFIXED_MAJOR andFIXED_MINOR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
They extract the two 16-bit values. However, there's a bug inFIXED_MAJOR; it masks the input as if it were 16.12 (with 4 undefined bits) yet shifts by 16. The top 4 bits get lost.
In the end though, it turns out we never use these values for anything (they're all versions specifiers.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, so they're what the freestype docs call something like FT_MAJOR and FT_MINOR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see such things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Those are the Freetype version; I'm referring to font file versions.
74ab0a6 to03325a5CompareIIRC, most ABI upcast values passed to vararg functions anyway, butthere might be some other ABIs that require the exact correct type.
It was masking the wrong bits, but fortunately, we never seem to usethis value for anything.
03325a5 toea69e03ComparePing@mdboom? |
1 similar comment
Ping@mdboom? |
| #define FIXED_MAJOR(val) (long)((val &0xffff000) >> 16) | ||
| #define FIXED_MINOR(val) (long)(val & 0xffff) | ||
| #define FIXED_MAJOR(val) (signed short)((val &0xffff0000) >> 16) | ||
| #define FIXED_MINOR(val) (unsigned short)(val & 0xffff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should also cause a type change in the version and fontrevision fields of converting TT_Header no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As inhere?
I think just a type change is missing? Otherwise looks fine (but this is the kind of things that decltype in modern C++ (e.g. via pybind11) makes much easier :-)). |
[MRG] Use exact types for Py_BuildValue.
IIRC, most ABI upcast values passed to vararg functions anyway, but there might be some other ABIs that require the exact correct type.