FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-96458: Statically initialize utf8 representation of static strings by kumaraditya303 · Pull Request #96481 · python/cpython · GitHub

Repository navigation

GH-96458: Statically initialize utf8 representation of static strings - #96481

Merged
gvanrossum merged 2 commits into
python:mainfrom
kumaraditya303:deepfreeze
Sep 3, 2022
Merged

gvanrossum merged 2 commits into
python:mainfrom
kumaraditya303:deepfreeze

Conversation

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

Copy link
Copy Markdown
Contributor

kumaraditya303 marked this pull request as ready for review September 1, 2022 17:16

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd like @ericsnowcurrently to review this too.

Copy link
Copy Markdown
Member

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

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.

ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

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.

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.

Copy link
Copy Markdown
Member

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

gvanrossum merged commit 6dab8c9 into python:main Sep 3, 2022
kumaraditya303 deleted the deepfreeze branch September 3, 2022 10:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

6 participants


Back | FazBrowse Home | New Git URL