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

Jump to bottom

bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ #30851

Merged
merged 2 commits into from Jan 26, 2022
Merged

bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ #30851

merged 2 commits into from Jan 26, 2022

Conversation

@tiran
Copy link
Member

@tiran tiran commented Jan 24, 2022 •
edited by miss-islington

https://bugs.python.org/issue46513

Automerge-Triggered-By: GH:tiran
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jan 24, 2022

🤖 New build scheduled with the buildbot fleet by @tiran for commit ebd6547 🤖

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

@hroncok
Copy link
Contributor

@hroncok hroncok commented Jan 25, 2022

https://bugs.python.org/issue46513

@tiran tiran changed the title Test build without AC_C_CHAR_UNSIGNED bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ Jan 25, 2022
@tiran tiran force-pushed the no_char_unsigned branch from ebd6547 to 881a1cf Jan 25, 2022
@tiran tiran marked this pull request as ready for review Jan 25, 2022
hroncok
hroncok approved these changes Jan 25, 2022
View changes
Copy link
Contributor

@hroncok hroncok left a comment

Thanks.

ronaldoussoren
ronaldoussoren reviewed Jan 25, 2022
View changes
Modules/audioop.c Outdated
/* This module currently does not work on systems where only unsigned
characters are available. Take it out of Setup. Sorry. */
Copy link
Contributor

@ronaldoussoren ronaldoussoren Jan 25, 2022

Choose a reason for hiding this comment

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

The last part of the comment should be removed, it is confusing without the preprocessor guards. I'd consider just dropping the comment, a compiler where "signed char" is not signed should be considered broken.

Copy link
Contributor

@arhadthedev arhadthedev Jan 25, 2022

Choose a reason for hiding this comment

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

The comment was introduced by b6775db in 1994, just five years after the first ever C standard was published. Internet distribution was slow and software companies were rigid back then so no surprise compatibility with per vendor compiler quirks was a real deal.

Copy link
Member

@vstinner vstinner Jan 25, 2022

Choose a reason for hiding this comment

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

Please remove this outdated comment. audioop explicitly uses signed char and unsigned char. Maybe to avoid any risk of confusion, the C code should use int8_t and uint8_t.

Copy link
Member Author

@tiran tiran Jan 25, 2022

Choose a reason for hiding this comment

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

The comment is pining for the fjords.

@miss-islington miss-islington merged commit 6e5a193 into python:main Jan 26, 2022
12 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jan 26, 2022

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jan 26, 2022

Sorry, @tiran, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6e5a193816e1bdf11f5fb78d620995fd6987ccf8 3.10

@miss-islington miss-islington self-assigned this Jan 26, 2022
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jan 26, 2022

Sorry @tiran, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 6e5a193816e1bdf11f5fb78d620995fd6987ccf8 3.9

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jan 26, 2022

GH-30914 is a backport of this pull request to the 3.10 branch.

tiran added a commit to tiran/cpython that referenced this issue Jan 26, 2022
@tiran
[3.10] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (pyth…
0f11817 
…onGH-30851)

(cherry picked from commit 6e5a193)

Co-authored-by: Christian Heimes <christian@python.org>
tiran added a commit to tiran/cpython that referenced this issue Jan 26, 2022
@tiran
[3.9] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (pytho…
247b9e4 
…nGH-30851)

(cherry picked from commit 6e5a193)

Co-authored-by: Christian Heimes <christian@python.org>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jan 26, 2022

GH-30915 is a backport of this pull request to the 3.9 branch.

tiran added a commit that referenced this issue Jan 26, 2022
@tiran
[3.10] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-3…
4371fbd 
…0851) (GH-30914)

Co-authored-by: Christian Heimes <christian@python.org>
tiran added a commit that referenced this issue Jan 26, 2022
@tiran
[3.9] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851
04772cd 
) (GH-30915)

Co-authored-by: Christian Heimes <christian@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

@ronaldoussoren ronaldoussoren

@arhadthedev arhadthedev

@encukou encukou

@hroncok hroncok

Assignees

@miss-islington miss-islington

Labels
CLA signed 🤖 automerge
Projects
None yet
Milestone
No milestone
9 participants
@tiran @bedevere-bot @hroncok @miss-islington @vstinner @encukou @ronaldoussoren @arhadthedev @the-knights-who-say-ni