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.

Sign up for GitHub

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-46443: deepfreeze use preallocated small ints #30715

Merged
merged 4 commits into from
Jan 20, 2022
Merged

bpo-46443: deepfreeze use preallocated small ints #30715

merged 4 commits into from
Jan 20, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jan 20, 2022 •
edited by bedevere-bot
Loading

@kumaraditya303 kumaraditya303 mentioned this pull request Jan 20, 2022
Closed
@kumaraditya303
Copy link
Contributor Author

It is an implementation detail so skip news.

@gvanrossum
Copy link
Member

It is an implementation detail so skip news.

I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

gvanrossum
gvanrossum reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG except I would like to see a one-line news item.

Did you check how much space this saves? (Probably nut that much since ints are small.)

Tools/scripts/deepfreeze.py Show resolved Hide resolved
@kumaraditya303
Copy link
Contributor Author

It is an implementation detail so skip news.

I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

Ok, I'll add a news fragment. Should I create a bpo for this now ?

@gvanrossum
Copy link
Member

Should I create a bpo for this now ?

Yeah.

@kumaraditya303 kumaraditya303 changed the title deepfreeze use preallocated small ints bpo-46443: deepfreeze use preallocated small ints Jan 20, 2022
@gvanrossum
Copy link
Member

I recommend that you put a little more effort in writing the initial comment for the bpo issues you create. E.g. not just what to do, but why it's worth doing and what impact you expect.

gvanrossum
gvanrossum approved these changes Jan 20, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! This was indeed super-simple. :-)

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jan 20, 2022 •
edited
Loading

I would also do this for empty bytes object as that also has a global singleton. (DONE)

@gvanrossum
Copy link
Member

I would also do this for empty bytes object as that also has a global singleton but separately.

Good idea. Maybe you can just add that to this PR, it's small enough.

@kumaraditya303
Copy link
Contributor Author

It is an implementation detail so skip news.

I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

I was confused by this PR as it skipped news #30092

gvanrossum
gvanrossum approved these changes Jan 20, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Great! Now just waiting for CI to finish...

@gvanrossum
Copy link
Member

I was confused by this PR as it skipped news #30092

Different core devs have different standards...

@gvanrossum gvanrossum merged commit 194ecc6 into python:main Jan 20, 2022
@gvanrossum
Copy link
Member

I'm the bot. :-)

@kumaraditya303 kumaraditya303 deleted the longobj branch January 20, 2022 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kumaraditya303 @gvanrossum @bedevere-bot