GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [2.7] bpo-10746: Fix ctypes PEP 3118 type codes for c_long, c_bool, c_int (GH-31) by pv · Pull Request #3242 · python/cpython

Conversation

@pv

@pv pv commented Aug 30, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Ctypes currently produces wrong pep3118 type codes for several types.
E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,
but it should be "<q" instead for sizeof(c_long) == 8

The problem is that the '<>' endian specification in the struct syntax
also turns on the "standard size" mode, which makes type characters have
a platform-independent meaning, which does not match with the codes used
internally in ctypes. The struct module format syntax also does not
allow specifying native-size non-native-endian items.

This commit adds a converter function that maps the internal ctypes
codes to appropriate struct module standard-size codes in the pep3118
format strings. The tests are modified to check for this..
(cherry picked from commit 07f1658)

https://bugs.python.org/issue10746

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pv
[2.7] bpo-10746: Fix ctypes PEP 3118 type codes for c_long, c_bool, c…
a8c7223 
…_int (pythonGH-31)

Ctypes currently produces wrong pep3118 type codes for several types.
E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,
but it should be "<q" instead for sizeof(c_long) == 8

The problem is that the '<>' endian specification in the struct syntax
also turns on the "standard size" mode, which makes type characters have
a platform-independent meaning, which does not match with the codes used
internally in ctypes.  The struct module format syntax also does not
allow specifying native-size non-native-endian items.

This commit adds a converter function that maps the internal ctypes
codes to appropriate struct module standard-size codes in the pep3118
format strings. The tests are modified to check for this..
(cherry picked from commit 07f1658)
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 30, 2017
@bedevere-bot bedevere-bot added the awaiting review label Aug 30, 2017
@pitrou

pitrou commented Aug 30, 2017

Copy link
Copy Markdown
Member

This PR failed building on AppVeyor, see: https://ci.appveyor.com/project/python/cpython/build/2.7.13+.5702#L306
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
eamanu
eamanu reviewed Aug 30, 2017
View reviewed changes
Comment thread Modules/_ctypes/_ctypes.c
#elif SIZEOF__BOOL == 8
case '?': pep_code = 'Q'; break;
#else
# error SIZEOF__BOOL has an unexpected value

@eamanu eamanu Aug 30, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hi @pv,

The problems is with the win compilation. SIZEOF__BOOL has an unexpected value

I'm really new, but that sound how if you don't define SIZEOF__BOOL variable. Searching where is that defining, I found its defintion in PC/pyconfig.h, but you never including that. I cannot test if that is the solution, because I don`t have my tools here.

regard!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Aug 30, 2017
@pv

pv commented Aug 30, 2017 via email

Copy link
Copy Markdown
Contributor Author
Thanks for the comment. I will look at this again next week once I'm back at a win32 machine. 30.8.2017 17.40 "Emmanuel Arias" <[email protected]> kirjoitti:
…
***@***.**** commented on this pull request. ------------------------------ In Modules/_ctypes/_ctypes.c <#3242 (comment)>: > +#elif SIZEOF_LONG == 8 + case 'l': pep_code = 'q'; break; + case 'L': pep_code = 'Q'; break; +#else +# error SIZEOF_LONG has an unexpected value +#endif /* SIZEOF_LONG */ +#if SIZEOF__BOOL == 1 + case '?': pep_code = '?'; break; +#elif SIZEOF__BOOL == 2 + case '?': pep_code = 'H'; break; +#elif SIZEOF__BOOL == 4 + case '?': pep_code = 'L'; break; +#elif SIZEOF__BOOL == 8 + case '?': pep_code = 'Q'; break; +#else +# error SIZEOF__BOOL has an unexpected value Hi @pv <https://github.com/pv>, The problems is with the win compilation. SIZEOF__BOOL has an unexpected value I'm really new, but sound how if you don't define SIZEOF__BOOL variable. Searching where is that defining, I found its defintion in PC/pyconfig.h, but you never including that. I cannot test if that is the solution, because I don`t have my tools here. regard! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3242 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5gLOsgdiCoDOfqW9CuTamaz9aRl7ks5sdWZBgaJpZM4PHCsl> .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pv
[2.7] bpo-10746: ensure SIZEOF__BOOL is defined
33b7e42
@pv

pv commented Sep 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Should be fixed now. In Python 2.7, SIZEOF__BOOL is defined to 1 if HAVE_C99_BOOL is undefined in cfield.c. I now duplicated that definition in ctypes.c. On Python 3.x, there are no such ifdefs for SIZEOF__BOOL in either file, as the variable seems to be always defined.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pitrou

pitrou commented Sep 2, 2017

Copy link
Copy Markdown
Member

Thank you! Now merging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pitrou
pitrou merged commit 990b2d0 into python:2.7 Sep 2, 2017
@Mariatta Mariatta removed the awaiting core review label Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@eamanu eamanu eamanu left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@pv @pitrou @eamanu @Mariatta @the-knights-who-say-ni @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.