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-96458: Statically initialize utf8 representation of static strings by kumaraditya303 · Pull Request #96481 · python/cpython

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Sep 1, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: _PyStaticUnicode_Dealloc should not exist. #96458

Sorry, something went wrong.

Uh oh!

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

All reactions
kumaraditya303 added 2 commits September 1, 2022 16:42
@kumaraditya303
static initialize utf8 representation
1ee2f54
@kumaraditya303
fix global objects
c15d97d
@bedevere-bot bedevere-bot added the awaiting review label Sep 1, 2022
@kumaraditya303 kumaraditya303 added the skip news label Sep 1, 2022
@kumaraditya303
kumaraditya303 marked this pull request as ready for review September 1, 2022 17:16
@kumaraditya303
kumaraditya303 requested a review from gvanrossum September 1, 2022 17:16
serhiy-storchaka
serhiy-storchaka reviewed Sep 1, 2022
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

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.

_PyUnicode_LATIN1_INIT is wrong name. You can statically initialize utf8 representation only in ASCII strings.

Sorry, something went wrong.

Uh oh!

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

All reactions
@kumaraditya303 kumaraditya303 mentioned this pull request Sep 2, 2022
_PyStaticUnicode_Dealloc should not exist. #96458
Closed
gvanrossum
gvanrossum approved these changes Sep 2, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

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'd like @ericsnowcurrently to review this too.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 2, 2022
@gvanrossum
gvanrossum requested a review from ericsnowcurrently September 2, 2022 16:47
@markshannon

markshannon commented Sep 2, 2022

Copy link
Copy Markdown
Member

Are you generating utf8 strings separately from the ascii string, or do they share the same data?

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Sep 2, 2022

Copy link
Copy Markdown
Member

Are you generating utf8 strings separately from the ascii string, or do they share the same data?

The utf8 cache strings are only ever created and used for non-ASCII strings (i.e. at least one character is 128 or larger). The unicode string object is smart enough to know that for pure ASCII strings the utf8 is identical so it doesn't bother creating or using the utf8 cache value.

This PR only generates utf8 data for exactly 6 strings in deepfreeze.c. I'm sure those are the ones with at least non-ASCII character.

In order to see a leak one would have to request the utf8 encoding of at least one of those 6 strings.

All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently
ericsnowcurrently approved these changes Sep 2, 2022
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

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.

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions
@ericsnowcurrently

ericsnowcurrently commented Sep 2, 2022

Copy link
Copy Markdown
Member

_PyUnicode_LATIN1_INIT is wrong name. You can statically initialize utf8 representation only in ASCII strings.

What do you mean? Keep in mind that the macro is used exclusively to statically initialize latin-1 strings.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Sep 2, 2022

Copy link
Copy Markdown
Member

_PyUnicode_LATIN1_INIT is wrong name. You can statically initialize utf8 representation only in ASCII strings.

What do you mean? Keep in mind that the macro is used exclusively to statically initialize latin-1 strings.

Indeed -- in fact the macro is only used in pycore_runtime_init_generated.h (a generated file :-) to initialize the _latin1 field of unicode objects. So the name seems eminently reasonable.

serhiy-storchaka 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.

serhiy-storchaka
serhiy-storchaka approved these changes Sep 3, 2022
View reviewed changes
@serhiy-storchaka

serhiy-storchaka commented Sep 3, 2022

Copy link
Copy Markdown
Member

Sorry, now I see that _PyUnicode_LATIN1_INIT takes a new argument.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum
gvanrossum merged commit 6dab8c9 into python:main Sep 3, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 3, 2022
@kumaraditya303
kumaraditya303 deleted the deepfreeze branch September 3, 2022 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes
@gvanrossum gvanrossum gvanrossum approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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

@kumaraditya303 @markshannon @gvanrossum @ericsnowcurrently @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.