Use exact types for Py_BuildValue. by QuLogic · Pull Request #7853 · matplotlib/matplotlib · GitHub
Skip to content

Use exact types for Py_BuildValue. - #7853

Merged
efiring merged 2 commits into
matplotlib:masterfrom
QuLogic:Py_BuildValue-types
Feb 25, 2018
Merged

efiring merged 2 commits into
matplotlib:masterfrom
QuLogic:Py_BuildValue-types

Conversation

@QuLogic

@QuLogic QuLogic commented Jan 17, 2017

Copy link
Copy Markdown
Member

IIRC, most ABI upcast values passed to vararg functions anyway, but there might be some other ABIs that require the exact correct type.

@QuLogic
QuLogic force-pushed the Py_BuildValue-types branch from 2d70522 to 74ab0a6 Compare January 19, 2017 02:23
@QuLogic QuLogic changed the title Use exact types for Py_BuildValue. [MRG] Use exact types for Py_BuildValue. Jan 21, 2017
@QuLogic
QuLogic requested a review from mdboom January 21, 2017 04:34
@story645

Copy link
Copy Markdown
Member

@QuLogic

QuLogic commented Jan 24, 2017

Copy link
Copy Markdown
Member Author

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 Py_BuildValue, so there's generally always enough space to treat it as a larger type even when it didn't start as one. I know for certain that this is true of float, which is promoted to double, on x86*, but I'm not exactly sure about integral types. And the ABI may be different on other architectures.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Jan 24, 2017
Comment thread src/ft2font.h Outdated
*/
#define FIXED_MAJOR(val) (long)((val & 0xffff000) >> 16)
#define FIXED_MINOR(val) (long)(val & 0xffff)
#define FIXED_MAJOR(val) (unsigned short)((val & 0xffff000) >> 16)

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are FIXED_MAJOR and FIXED_MINOR?

Copy link
Copy Markdown
Member Author

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 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.)

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

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.

@QuLogic
QuLogic force-pushed the Py_BuildValue-types branch from 74ab0a6 to 03325a5 Compare January 27, 2017 09:08
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.
@QuLogic
QuLogic force-pushed the Py_BuildValue-types branch from 03325a5 to ea69e03 Compare January 28, 2017 03:26
@QuLogic

QuLogic commented May 12, 2017

Copy link
Copy Markdown
Member Author

Ping @mdboom?

1 similar comment
@QuLogic

QuLogic commented Aug 23, 2017

Copy link
Copy Markdown
Member Author

Ping @mdboom?

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Sep 24, 2017
@QuLogic
QuLogic requested a review from anntzer February 5, 2018 04:03
Comment thread src/ft2font.h
#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)

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in here?

@anntzer

anntzer commented Feb 10, 2018

Copy link
Copy Markdown
Contributor

@tacaswell tacaswell modified the milestones: v2.2, v3.0 Feb 10, 2018
@efiring
efiring merged commit 57c9a78 into matplotlib:master Feb 25, 2018
@QuLogic
QuLogic deleted the Py_BuildValue-types branch February 25, 2018 21:44
@QuLogic QuLogic changed the title [MRG] Use exact types for Py_BuildValue. Use exact types for Py_BuildValue. Feb 25, 2018
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Oct 19, 2019
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.

6 participants