-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix some more integer type inconsistencies in Freetype code #7782
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
Conversation
This straightens up some more inconsistencies in the integer types in ft2font_wrapper.cpp and ft2font.cpp. Referring to the Freetype 2 API guide: https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html 1) FT_Get_Kerning's integer arguments are FT_UInt (unsigned int) but we were passing it signed ints. 2) FT_Load_Glyph's flags argument is an FT_Int32 (signed int... usually, see footnote) but our set_text and load_glyph were using FT_UInt32, and converting Python values to unsigned int. 3) FT_Load_Char's flags argument is an FT_Int32 (signed int... usually, see footnote) but our load_char was using FT_UInt32, and converting Python values to unsigned int. 4) load_char in ft2font_wrapper declared charcode as a signed long (and load_char in ft2font does indeed expect a signed long from the wrapper, though it then turns it into an unsigned long, I don't know why this is set up that way), but was converting the Python value to an unsigned long (k) not a signed long (l). 5) get_glyph_name in ft2font_wrapper declared glyph_number as unsigned int, and indeed ft2font is expecting an unsigned int (Freetype is expecting an FT_UInt, which is the same thing), but it was converting the Python value to a signed int (i) not an unsigned int (I). Footnote: the FT_Int32 and FT_UInt32 types that we have to use for some values are defined as being the signed and unsigned variants of whichever integer type is *exactly* 32 bits in size. Freetype defines them as int if it's 32 bits, otherwise long if *that's* 32 bits, otherwise it gives up and errors out. However, we simply assume they're always ints, which isn't technically correct. We could probably fix this with a bit of `sizeof()` use, but I'm not sure if we want to bother, because it's almost certainly the case that the int types are 32 bits in size on all platforms matplotlib is targeting - if there are still any significant platforms where int is 16 bits and so these wind up as long, I'd be surprised. I thought it was worth noting, though.
Current coverage is 62.12% (diff: 100%)@@ master #7782 diff @@
==========================================
Files 174 174
Lines 56028 56028
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 34805 34805
Misses 21223 21223
Partials 0 0
|
👍 |
Backported to v2.x as 7914b86 |
@AdamWill: Thanks for all of this! We worked on bigendian back in the day when PPC Macs were a thing, but obviously we haven't kept up with it! |
Fix some more integer type inconsistencies in Freetype code
Oh, I did run these through the test suite and they didn't change any of the results, so I don't think these changes are super critical, it seems like these inconsistencies don't cause any observable problems. |
This straightens up some more inconsistencies in the integer
types in ft2font_wrapper.cpp and ft2font.cpp. Referring to the
Freetype 2 API guide:
https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html
FT_Get_Kerning's integer arguments are FT_UInt (unsigned int)
but we were passing it signed ints.
FT_Load_Glyph's flags argument is an FT_Int32 (signed int...
usually, see footnote) but our set_text and load_glyph were
using FT_UInt32, and converting Python values to unsigned int.
FT_Load_Char's flags argument is an FT_Int32 (signed int...
usually, see footnote) but our load_char was using FT_UInt32,
and converting Python values to unsigned int.
load_char in ft2font_wrapper declared charcode as a signed
long (and load_char in ft2font does indeed expect a signed long
from the wrapper, though it then turns it into an unsigned
long, I don't know why this is set up that way), but was
converting the Python value to an unsigned long (k) not a
signed long (l).
get_glyph_name in ft2font_wrapper declared glyph_number as
unsigned int, and indeed ft2font is expecting an unsigned int
(Freetype is expecting an FT_UInt, which is the same thing),
but it was converting the Python value to a signed int (i)
not an unsigned int (I).
Footnote: the FT_Int32 and FT_UInt32 types that we have to use
for some values are defined as being the signed and unsigned
variants of whichever integer type is exactly 32 bits in
size. Freetype defines them as int if it's 32 bits, otherwise
long if that's 32 bits, otherwise it gives up and errors out.
However, we simply assume they're always ints, which isn't
technically correct. We could probably fix this with a bit of
sizeof()
use, but I'm not sure if we want to bother, becauseit's almost certainly the case that the int types are 32 bits
in size on all platforms matplotlib is targeting - if there are
still any significant platforms where int is 16 bits and so these
wind up as long, I'd be surprised. I thought it was worth noting,
though.
I've not yet tested these changes separately from the changes in #7781 to see if they actually fix any real issues in the test suite, but I think they're at least technically correct.