Skip to content

bpo-46443: deepfreeze use preallocated small ints - #30715

Merged
gvanrossum merged 4 commits into
python:mainfrom
kumaraditya303:longobj
Jan 20, 2022
Merged

bpo-46443: deepfreeze use preallocated small ints#30715
gvanrossum merged 4 commits into
python:mainfrom
kumaraditya303:longobj

Conversation

@kumaraditya303

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

Copy link
Copy Markdown
Contributor

@kumaraditya303kumaraditya303 mentioned this pull request Jan 20, 2022
Closed
@kumaraditya303

kumaraditya303 commented Jan 20, 2022

Copy link
Copy Markdown
ContributorAuthor

It is an implementation detail so skip news.

@gvanrossum

gvanrossum commented Jan 20, 2022

Copy link
Copy Markdown
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

@gvanrossumgvanrossum 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.

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.)

Comment threadTools/scripts/deepfreeze.py
@kumaraditya303

kumaraditya303 commented Jan 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

gvanrossum commented Jan 20, 2022

Copy link
Copy Markdown
Member

Should I create a bpo for this now ?

Yeah.

@kumaraditya303kumaraditya303 changed the title deepfreeze use preallocated small intsbpo-46443: deepfreeze use preallocated small intsJan 20, 2022
@gvanrossum

gvanrossum commented Jan 20, 2022

Copy link
Copy Markdown
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

@gvanrossumgvanrossum 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.

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

@kumaraditya303

kumaraditya303 commented Jan 20, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@gvanrossum

gvanrossum commented Jan 20, 2022

Copy link
Copy Markdown
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

kumaraditya303 commented Jan 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@gvanrossumgvanrossum 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.

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

@gvanrossum

gvanrossum commented Jan 20, 2022

Copy link
Copy Markdown
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:mainJan 20, 2022
@gvanrossum

gvanrossum commented Jan 20, 2022

Copy link
Copy Markdown
Member

I'm the bot. :-)

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

Reviewers

@gvanrossumgvanrossumgvanrossum 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