GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-1635741: Port _collections module to multiphase initialization. by corona10 · Pull Request #19074 · python/cpython

Conversation

corona10 commented Mar 19, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 added 2 commits

March 19, 2020 23:45


          bpo-1635741: Port _collections module to multiphase initialization.

fb35d81


          bpo-1635741: fix

ac5d78b

corona10 requested a review from rhettinger as a code owner

March 19, 2020 15:21

the-knights-who-say-ni added the CLA signed label

Mar 19, 2020

bedevere-bot added the awaiting review label

Mar 19, 2020

corona10 commented Mar 19, 2020 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member Author

For the record, I will work on a new helper function after this PR is landed if this work is worth to do.
#19071 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

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

corona10 requested a review from vstinner

March 19, 2020 15:40

corona10 commented Mar 19, 2020

Copy link

Copy Markdown

Member Author

@shihai1991 Please take a look :)

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner approved these changes

Mar 19, 2020

View reviewed changes

vstinner 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 apart "m" variable name which looks too short to me ;-)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_collectionsmodule.c

		if (PyType_Ready(type) < 0) {
		return -1;
		}
		const char *name = _PyType_Name(type);

vstinner Mar 19, 2020

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.

FYI I checked manually that the actual type name and _PyType_Name() gives the same name than before this change.

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 Mar 19, 2020

Copy link

Copy Markdown

Member Author

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 did same thing also :) Thank you for the re-checking!

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_collectionsmodule.c Outdated

		}
		const char *name = _PyType_Name(type);
		Py_INCREF(type);
		if (PyModule_AddObject(m, name, (PyObject *)type) < 0) {

vstinner Mar 19, 2020

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.

Checking for PyModule_AddObject() error is a good thing ;-)

Sorry, something went wrong.

Uh oh!

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

corona10 and shihai1991 reacted with thumbs up emoji

All reactions

👍 2 reactions

Comment thread

Modules/_collectionsmodule.c Outdated

		};

		static int
		collections_exec(PyObject *m) {

vstinner Mar 19, 2020

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.

nitpick: would you mind to rename it to "mod" or "module"?

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 Mar 19, 2020

Copy link

Copy Markdown

Member Author

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.

module might be great! Thanks for the suggestion

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting merge and removed awaiting review labels

Mar 19, 2020


          bpo-1635741: Apply Victor's review

08612b0

shihai1991 commented Mar 19, 2020

Copy link

Copy Markdown

Member

oh, on~ my network crashed at this point :(
I can not review codes now, waiting a moment~

All reactions

Sorry, something went wrong.

Uh oh!

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

shihai1991 reviewed

Mar 19, 2020

View reviewed changes

Comment thread

Modules/_collectionsmodule.c


		defdict_type.tp_base = &PyDict_Type;

		for (int i = 0; typelist[i] != NULL; i++) {

shihai1991 Mar 19, 2020

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.

nice improvment.

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 approved these changes

Mar 19, 2020

View reviewed changes

shihai1991 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 like the improvment of typelist.
LGTM :)

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 commented Mar 19, 2020

Copy link

Copy Markdown

Member Author

@vstinner @shihai1991

Thanks for the reviews :)

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner merged commit 77248a2 into python:master

Mar 19, 2020

bedevere-bot removed the awaiting merge label

Mar 19, 2020

corona10 deleted the bpo-1635741-_collectionsmodule branch

March 31, 2020 12:49

kylotan mannequin mentioned this pull request

Sep 19, 2022

Py_Finalize() doesn't clear all Python objects at exit #44470

Closed

Reviewers

vstinner

vstinner approved these changes

shihai1991

shihai1991 approved these changes

rhettinger

Awaiting requested review from rhettinger

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

5 participants

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.

GeezerVison

Conversation

corona10 commented Mar 19, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Mar 19, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 commented Mar 19, 2020

Uh oh!

shihai1991 Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 commented Mar 19, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

corona10 commented Mar 19, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

corona10 commented Mar 19, 2020 •

edited

Loading

Uh oh!

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