Use exact types for Py_BuildValue. #7853
Conversation
2d70522 to
74ab0a6
Compare
|
Stupid 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 the FreeType 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.
Isn't that supposed to be a signed long?
There was a problem hiding this comment.
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.
What are FIXED_MAJOR and FIXED_MINOR?
There was a problem hiding this comment.
They extract the two 16-bit values. However, there's a bug in FIXED_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.
Oh, so they're what the freestype docs call something like FT_MAJOR and FT_MINOR?
There was a problem hiding this comment.
I don't see such things?
There was a problem hiding this comment.
There was a problem hiding this comment.
Those are the Freetype version; I'm referring to font file versions.
74ab0a6 to
03325a5
Compare
IIRC, most ABI upcast values passed to vararg functions anyway, but there might be some other ABIs that require the exact correct type.
It was masking the wrong bits, but fortunately, we never seem to use this value for anything.
03325a5 to
ea69e03
Compare
|
Ping @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.
Should also cause a type change in the version and fontrevision fields of converting TT_Header no?
|
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.