GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40170: Convert PyObject_IS_GC() macro to a function by shihai1991 · Pull Request #19464 · python/cpython

Conversation

@shihai1991

@shihai1991 shihai1991 commented Apr 10, 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/issue40170

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shihai1991
Convert PyObject_IS_GC() macro to a function
8bb1fab
@shihai1991
shihai1991 requested a review from pablogsal as a code owner April 10, 2020 22:35
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 10, 2020
@bedevere-bot bedevere-bot added the awaiting review label Apr 10, 2020
@shihai1991
Update 2020-04-11-06-12-44.bpo-40170.cmM9oK.rst
be2bbab 
Fix typo of news
pablogsal
pablogsal approved these changes Apr 10, 2020
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Apr 10, 2020
@pablogsal

pablogsal commented Apr 11, 2020

Copy link
Copy Markdown
Member

@shihai1991 Could you rebase the PR to pick the latest changes?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
resolve conflict
2cbeedb
@shihai1991

shihai1991 commented Apr 11, 2020

Copy link
Copy Markdown
Member Author

@shihai1991 Could you rebase the PR to pick the latest changes?

Got it, done ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal requested changes Apr 11, 2020
View reviewed changes

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

Hummmm, I think we still need a macro version for internal use for performance reasons. @vstinner was that your original intent?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 11, 2020
@bedevere-bot

bedevere-bot commented Apr 11, 2020

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Apr 11, 2020
@vstinner

vstinner commented Apr 13, 2020

Copy link
Copy Markdown
Member

Hummmm, I think we still need a macro version for internal use for performance reasons.

Right, please add a static inline function _PyObject_IS_GC() in pycore_object.h and modify gcmodule.c to use it (replace PyObject_IS_GC with _PyObject_IS_GC).

PyObject_IS_GC() function body should just call _PyObject_IS_GC().
shihai1991 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
shihai1991 added 2 commits April 13, 2020 14:04
@shihai1991
add _PyObject_IS_GC()
0783d0b
@shihai1991
Resolve confilcts
32c0acc
@shihai1991

shihai1991 commented Apr 13, 2020

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Apr 13, 2020
@bedevere-bot

bedevere-bot commented Apr 13, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from pablogsal April 13, 2020 12:45
vstinner
vstinner reviewed Apr 14, 2020
View reviewed changes
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
update docs
c69d697
vstinner
vstinner reviewed Apr 14, 2020
View reviewed changes
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/gcmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/cpython/objimpl.h Outdated
Comment on lines +130 to +133
if (PyType_IS_GC(Py_TYPE(obj))) {
return (Py_TYPE(obj)->tp_is_gc == NULL || Py_TYPE(obj)->tp_is_gc(obj));
}
return 0;

@vstinner vstinner Apr 14, 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.

Suggested change
if (PyType_IS_GC(Py_TYPE(obj))) {
return (Py_TYPE(obj)->tp_is_gc == NULL || Py_TYPE(obj)->tp_is_gc(obj));
}
return 0;
return (PyType_IS_GC(Py_TYPE(obj))
&& (Py_TYPE(obj)->tp_is_gc == NULL
|| Py_TYPE(obj)->tp_is_gc(obj)));

Sorry, something went wrong.

Uh oh!

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

@shihai1991 shihai1991 Apr 14, 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.

this code style looks more simple?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/gcsupport.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
shihai1991 and others added 4 commits April 14, 2020 22:53
@shihai1991 @vstinner
Update Doc/c-api/gcsupport.rst
1dd8dd2 
Co-Authored-By: Victor Stinner <[email protected]>
@shihai1991 @vstinner
Update Doc/c-api/gcsupport.rst
176f8a0 
Co-Authored-By: Victor Stinner <[email protected]>
@shihai1991
update some descriptions
8d68011
@shihai1991
update docs
ee0525b
@shihai1991
fix typos
a4d1a37
vstinner
vstinner reviewed Apr 14, 2020
View reviewed changes
Comment thread Include/cpython/objimpl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/cpython/objimpl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
shihai1991 added 2 commits April 15, 2020 00:00
@shihai1991
remove _PyObject_IS_GC() to pycore_object.h
bdc61b9
@shihai1991
remove white space
1c242b5
vstinner
vstinner reviewed Apr 14, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/C API/2020-04-11-06-12-44.bpo-40170.cmM9oK.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
update news
cffba90
@vstinner
vstinner merged commit 675d9a3 into python:master Apr 14, 2020
@bedevere-bot bedevere-bot removed the awaiting change review label Apr 14, 2020
@vstinner

vstinner commented Apr 14, 2020

Copy link
Copy Markdown
Member

Thanks @shihai1991!
shihai1991 reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

shihai1991 commented Apr 15, 2020

Copy link
Copy Markdown
Member Author

Thanks @shihai1991!

Thanks, folks. I learned much in this PR(especially in docs(what is user really need).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@pablogsal pablogsal Awaiting requested review from pablogsal

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

@shihai1991 @pablogsal @bedevere-bot @vstinner @the-knights-who-say-ni
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 01402520 — 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.