GeezerVison

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

GeezerVison is showing github.com — bpo-40170: Hide implementation detail of Py_TRASHCAN_BEGIN macro by shihai1991 · Pull Request #23235 · python/cpython

Conversation

shihai1991 commented Nov 11, 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


          Use PyType_GetSlot() in Py_TRASHCAN_BEGIN macro.

8ab6dc8

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

Nov 11, 2020

bedevere-bot added the awaiting review label

Nov 11, 2020

shihai1991 commented Nov 11, 2020

Copy link

Copy Markdown

Member Author

cc @vstinner

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Nov 11, 2020

View reviewed changes

Comment thread

Include/cpython/object.h Resolved

Uh oh!

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


          add the _PyTrash_cond() function

b288c3e

shihai1991 changed the title ~~bpo-40170: Use PyType_GetSlot() in Py_TRASHCAN_BEGIN macro.~~ bpo-40170: Hide implementation detail of Py_TRASHCAN_BEGIN macro

Nov 12, 2020

vstinner reviewed

Nov 12, 2020

View reviewed changes

Comment thread

Include/cpython/object.h Resolved

Uh oh!

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

Comment thread

Include/cpython/object.h Resolved

Uh oh!

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

Comment thread

Doc/whatsnew/3.10.rst Outdated

		* The ``Py_TRASHCAN_BEGIN`` macro no longer accesses PyTypeObject attributes,
		but now can get the condition by calling the new private
		:c:func:`_PyTrash_cond()` function which hides implementation details.
		(Contributed by Hai Shi in :issue:`40170`.)

vstinner Nov 12, 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.

I don't think that it's worth it to mention in What's New in Python 3.10. End users will not notice and don't care ;-)

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 Nov 13, 2020 •

edited

Loading

Uh oh!

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

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.

Ok, removed.
I decided add this news because I saw you have written the description info of _PyTrash_begin() and _PyTrash_end() in whatsnew. Lol~

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Nov 13, 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.

I wrote a changelog entry, but nothing in What's New in Python 3.9 or What's New in Python 3.10, no?

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 Nov 13, 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.

Oh, I saw this info in https://github.com/python/cpython/blob/master/Misc/NEWS.d/3.9.0a5.rst.
Looks like I confused the news file with relese file : (, sorry.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Nov 13, 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.

No problem.

I consider that "What's New in Python X.Y" should be short and only describe the public API, but the Changelog should describe every single change and private functions can be mentioned there.

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 Nov 13, 2020 •

edited

Loading

Uh oh!

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

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.

Oh, This great. I am very confused when I start write the whatsnew file in the first time.
It's more like a double copy operation.

In fact, I don't know how release manager to archieve those change log.
My simple thought: Maybe we can create some labels to tag what's info should be added in release file or not. When release manager archive those change log, those taged changeinfo chould be archieved in release file automatically.

If this thought have any value, I can add this in my TODO list. Lol~

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Include/cpython/object.h Resolved

Uh oh!

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


          apply victor's comment

a0c82d7

vstinner reviewed

Nov 13, 2020

View reviewed changes

Comment thread

Objects/object.c Resolved

Uh oh!

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


          apply victor's comment

3d2f872

vstinner reviewed

Nov 13, 2020

View reviewed changes

Comment thread

Objects/object.c Resolved

Uh oh!

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


          apply victor's comment

db2d98c

shihai1991 commented Nov 24, 2020

Copy link

Copy Markdown

Member Author

@vstinner Hi, victor. Is this PR good enough to be merged?

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner approved these changes

Nov 24, 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, but please address my last minor request (just add a comment).

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/object.c

		}


		int

vstinner Nov 24, 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.

Please add comment explaining that it is used by the Py_TRASHCAN_BEGIN macro.

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 Nov 24, 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.

Got it, updated.

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

Nov 24, 2020


          apply victor's comment

477dbd8

vstinner merged commit ed1a5a5 into python:master

Nov 24, 2020

bedevere-bot removed the awaiting merge label

Nov 24, 2020

vstinner commented Nov 24, 2020

Copy link

Copy Markdown

Member

Merged, thanks.

All reactions

Sorry, something went wrong.

Uh oh!

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

shihai1991 commented Nov 25, 2020

Copy link

Copy Markdown

Member Author

Thanks, victor.

All reactions

Sorry, something went wrong.

Uh oh!

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

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 13, 2021


          bpo-40170: Hide impl detail of Py_TRASHCAN_BEGIN macro (pythonGH-23235)

14bfdb2

The Py_TRASHCAN_BEGIN macro no longer accesses PyTypeObject attributes,
but now can get the condition by calling the new private
_PyTrash_cond() function which hides implementation details.

Reviewers

vstinner

vstinner approved these changes

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.

4 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

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

Uh oh!

Uh oh!

shihai1991 commented Nov 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shihai1991 commented Nov 24, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 24, 2020

Uh oh!

shihai1991 commented Nov 25, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shihai1991 commented Nov 11, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

shihai1991 Nov 13, 2020 •

edited

Loading

Uh oh!

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

shihai1991 Nov 13, 2020 •

edited

Loading

Uh oh!

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