Fix some more integer type inconsistencies in Freetype code#7782
Merged
mdboom merged 1 commit intomatplotlib:masterfrom Jan 11, 2017
Merged
Fix some more integer type inconsistencies in Freetype code#7782mdboom merged 1 commit intomatplotlib:masterfrom
mdboom merged 1 commit intomatplotlib:masterfrom
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
|
Member
|
👍 |
Member
|
Backported to v2.x as 7914b86 |
Member
|
@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! |
mdboom
added a commit
that referenced
this pull request
Jan 11, 2017
Fix some more integer type inconsistencies in Freetype code
Contributor
Author
|
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.