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 — gh-85858: Remove PyUnicode_InternImmortal() function by vstinner · Pull Request #92579 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented May 9, 2022

Copy link
Copy Markdown
Member

Remove the PyUnicode_InternImmortal() function and the
SSTATE_INTERNED_IMMORTAL macro.

The PyUnicode_InternImmortal() function is still exported in the
stable ABI. The function is removed from the API.

PyASCIIObject.state.interned size is now a single bit, rather than 2
bits.

Keep SSTATE_NOT_INTERNED and SSTATE_INTERNED_MORTAL macros for
backward compatibility, but no longer use them internally since the
interned member is now a single bit and so can only have two values
(interned or not interned).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting core review label May 9, 2022
@vstinner

vstinner commented May 9, 2022

Copy link
Copy Markdown
Member Author

@methane @serhiy-storchaka: Would you mind to review this change?

I reused #85858 issue.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed May 10, 2022
View reviewed changes
Comment thread Include/cpython/unicodeobject.h Outdated

@serhiy-storchaka serhiy-storchaka May 10, 2022

Copy link
Copy Markdown
Member

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.

I would keep it 2-bit for a time.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner May 11, 2022

Copy link
Copy Markdown
Member Author

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.

PyASCIIObject is not part of the stable ABI. Why do you care about keeping 2 bits for interned?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/unicodeobject.h Outdated

@serhiy-storchaka serhiy-storchaka May 10, 2022

Copy link
Copy Markdown
Member

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.

Is not 2 versions too short period for removing from the public C API? I think we should be more conservative in such cases and prolong the deprecation to at least 4 years.

I would keep it as an alias of PyUnicode_InternInPlace which may print a deprecation warning.

Sorry, something went wrong.

Uh oh!

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

@methane methane May 10, 2022

Copy link
Copy Markdown
Member

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.

There are no use from top4000 packages.
https://github.com/hpyproject/top4000-pypi-packages/search?q=PyUnicode_InternImmortal

We can remove it in early alpha and resurrect it if some packages are broken by this.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner May 11, 2022

Copy link
Copy Markdown
Member Author

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.

Is not 2 versions too short period for removing from the public C API?

I followed PEP 387 process.

I think we should be more conservative in such cases and prolong the deprecation to at least 4 years.

Why do you care of this function in specific? Are you aware of projects using it?

There are no use from top4000 packages.

Same when I looked in December 2021:
#85858 (comment)

History of this function: #85858 (comment)

We can remove it in early alpha and resurrect it if some packages are broken by this.

Sure. I did that multiple times and I'm fine with reverting if it causes too many issues.

Removing the function early in 3.12 dev cycle gives more time to users to detect if their project is affected, to give them more time to decide how to update their code, or to ask to revert the change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
methane
methane approved these changes May 10, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 10, 2022
@vstinner
gh-85858: Remove PyUnicode_InternImmortal() function
b3f00dc 
Remove the PyUnicode_InternImmortal() function and the
SSTATE_INTERNED_IMMORTAL macro.

The PyUnicode_InternImmortal() function is still exported in the
stable ABI. The function is removed from the API.

PyASCIIObject.state.interned size is now a single bit, rather than 2
bits.

Keep SSTATE_NOT_INTERNED and SSTATE_INTERNED_MORTAL macros for
backward compatibility, but no longer use them internally since the
interned member is now a single bit and so can only have two values
(interned or not interned).

Update stats of _PyUnicode_ClearInterned().
@vstinner

vstinner commented May 12, 2022

Copy link
Copy Markdown
Member Author

I rebased my PR to fix merge conflicts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 059b5ba into python:main May 13, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label May 13, 2022
@vstinner
vstinner deleted the remove_immortal branch May 13, 2022 11:40
@vstinner

vstinner commented May 13, 2022

Copy link
Copy Markdown
Member Author

I merged my PR. Thanks for your review @methane.

@serhiy-storchaka: I understand your concerns. I tried to reply to them. I rely on @methane's approval to merge my change ;-)
All reactions

Sorry, something went wrong.

Uh oh!

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

SnoopJ commented Aug 24, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Do I understand correctly that this changeset alters the alignment of PyASCIIObject and derived types? I was just looking around this part of CPython and noticed that the embedded struct has a size of 31 bits (= 1 + 3 + 1 + 1 + 25), where it used to be 32 bits (= 2 + 3 + 1 + 1 + 25).

I'm not at all familiar with the m68k issue that caused the addition of the padding field (nor what the implications might be on target platforms), but thought that I would ask for confirmation if the adjustment to the width of interned here without a corresponding adjustment to the padding was deliberate.

Edit: pondering it some more, I may have asked a very silly question here, since the difference of one bit isn't going to make a difference in terms of bytes, there will "just" be this odd bit not formally accounted for.
All reactions

Sorry, something went wrong.

Uh oh!

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

methane commented Aug 25, 2022

Copy link
Copy Markdown
Member

@SnoopJeDi Nice catch.
FYI, 31bit is enough to guarantee 32bit alignment, so this nit doesn't cause any real bug.
SnoopJ reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
methane added a commit to methane/cpython that referenced this pull request Aug 25, 2022
@methane
Unicode: Fix padding in PyASCIIObject
4116170 
This padding was not updated when pythonGH-92579 reduced intern flag bits.
@methane methane mentioned this pull request Aug 25, 2022
gh-85858: c-api: Fix padding in PyASCIIObject #96262
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@methane methane methane approved these changes

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.

5 participants

@vstinner @SnoopJ @methane @serhiy-storchaka @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.