Skip to content
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

gh-82616: Add Py_IS_TYPE_SIGNED() macro #93178

Merged
merged 2 commits into from
May 27, 2022
Merged

gh-82616: Add Py_IS_TYPE_SIGNED() macro #93178

merged 2 commits into from
May 27, 2022

Conversation

vstinner
Copy link
Member

_posixsubprocess: add a static assertion to ensure that the pid_t
type is signed.

Replace _Py_IntegralTypeSigned() with Py_IS_TYPE_SIGNED().

@vstinner
Copy link
Member Author

@serhiy-storchaka
Copy link
Member

I did not know about _Py_IntegralTypeSigned(). It could be used in Modules/posixmodule.c and other interfaces to OS.

But I hesitate about adding it to the public API. Compilers can complain about "always true/false" condition. And what to do with this in PyPy and bindings in other programming languages?

1 similar comment
@serhiy-storchaka
Copy link
Member

I did not know about _Py_IntegralTypeSigned(). It could be used in Modules/posixmodule.c and other interfaces to OS.

But I hesitate about adding it to the public API. Compilers can complain about "always true/false" condition. And what to do with this in PyPy and bindings in other programming languages?

@vstinner
Copy link
Member Author

Ok, I add it as a private macro instead.

@vstinner
Copy link
Member Author

Oh, the Ubtuntu job logs a compiler warning:

comparison of unsigned expression < 0 is always false [-Wtype-limits]

vstinner added 2 commits May 27, 2022 00:47
_posixsubprocess: add a static assertion to ensure that the pid_t
type is signed.

Replace _Py_IntegralTypeSigned() with _Py_IS_TYPE_SIGNED().
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Neat trick to get rid of the compiler warning! LGTM, but I'd run it through the buildbots one more time before merging.

@vstinner vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 786c514 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
@vstinner
Copy link
Member Author

buildbot/PPC64 Fedora PR — Build done.

Unrelated error: ../configure: line 2692: cannot create temp file for here-document: No space left on device

@vstinner vstinner merged commit 22b75d9 into python:main May 27, 2022
@vstinner vstinner deleted the is_type_signed branch May 27, 2022 13:05
@vstinner
Copy link
Member Author

Merged, thanks for the reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants