Skip to content

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

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

AdamWill
Copy link
Contributor

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.

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.

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.
@codecov-io
Copy link

codecov-io commented Jan 10, 2017

Current coverage is 62.12% (diff: 100%)

Merging #7782 into master will not change coverage

@@             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          

Powered by Codecov. Last update 109251f...61f256b

@tacaswell tacaswell requested a review from mdboom January 10, 2017 14:01
@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Jan 10, 2017
@mdboom
Copy link
Member

mdboom commented Jan 11, 2017

👍

@mdboom mdboom merged commit 9b1ece0 into matplotlib:master Jan 11, 2017
@mdboom
Copy link
Member

mdboom commented Jan 11, 2017

Backported to v2.x as 7914b86

@mdboom
Copy link
Member

mdboom commented Jan 11, 2017

@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
@tacaswell tacaswell modified the milestones: 2.0 (style change major release), 2.0.1 (next bug fix release) Jan 11, 2017
@AdamWill
Copy link
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.

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.

4 participants