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-34100: Merge constants recursively #8341

Merged
merged 8 commits into from
Nov 26, 2018
Merged

bpo-34100: Merge constants recursively #8341

merged 8 commits into from
Nov 26, 2018

Conversation

methane
Copy link
Member

@methane methane commented Jul 19, 2018 •
edited by bedevere-bot
Loading

There are some same consts in a module. This commit merges them into
single instance. It reduces number of objects in memory after loading modules.

https://bugs.python.org/issue34100

asottile
asottile reviewed Aug 12, 2018
View reviewed changes
Copy link
Contributor

@asottile asottile left a comment

Choose a reason for hiding this comment

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

didn't really review the code -- sorry about the drive-by!

@methane methane closed this Sep 13, 2018
@methane methane reopened this Sep 13, 2018
@methane methane changed the title bpo-34100: compile: Merge constants bpo-34100: Merge constants recursively Sep 13, 2018
@methane methane closed this Sep 20, 2018
@methane methane reopened this Sep 20, 2018
vstinner
vstinner reviewed Oct 1, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I like the overall idea. But I don't have the bandwidth to review reference counting and other subtle details.

vstinner
vstinner approved these changes Nov 15, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

cc @serhiy-storchaka

@methane methane closed this Nov 16, 2018
@methane methane reopened this Nov 16, 2018
@methane methane closed this Nov 26, 2018
@methane methane reopened this Nov 26, 2018
vstinner
vstinner reviewed Nov 26, 2018
View reviewed changes
vstinner
vstinner approved these changes Nov 26, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@methane
Copy link
Member Author

methane commented Nov 26, 2018

Thank you for review.

... Travis seems very unresponsible...

@methane methane closed this Nov 26, 2018
@methane methane reopened this Nov 26, 2018
@miss-islington
Copy link
Contributor

@methane: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit c2e1607 into python:master Nov 26, 2018
@methane methane deleted the merge-consts branch November 29, 2018 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asottile asottile asottile left review comments

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

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

Successfully merging this pull request may close these issues.
6 participants
@methane @miss-islington @vstinner @asottile @the-knights-who-say-ni @bedevere-bot