GeezerVison

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

GeezerVison is showing github.com — bpo-1635741: sqlite3 now uses Py_NewRef and Py_XNewRef by erlend-aasland · Pull Request #23170 · python/cpython

Conversation

erlend-aasland commented Nov 5, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Thanks for GH-23152, @vstinner! Fewer lines, improved readability: 7 files changed, 40 insertions, 87 deletions :)
Would you mind reviewing?

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

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

All reactions


          Use Py_NewRef and Py_XNewRef in sqlite3

dc6486d

erlend-aasland requested a review from berkerpeksag as a code owner

November 5, 2020 18:51

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

Nov 5, 2020

bedevere-bot added the awaiting review label

Nov 5, 2020

erlend-aasland commented Nov 5, 2020

Copy link

Copy Markdown

Contributor Author

FYI, CI hangs at test_tag_bind (tkinter.test.test_ttk.test_widgets.TreeviewTest) ... Timeout (0:20:00)! Kick CI again?

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Nov 6, 2020

Copy link

Copy Markdown

Member

FYI, CI hangs at test_tag_bind (tkinter.test.test_ttk.test_widgets.TreeviewTest) ... Timeout (0:20:00)! Kick CI again?

You can ignore this known issue https://bugs.python.org/issue42142 The core dev who will merge your PR wil re-run the job.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Nov 6, 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.

I like node->key = Py_NewRef(key);, it looks nice.

But I'm not sure about long lines like current_param = Py_XNewRef(PyDict_GetItemWithError(parameters, binding_name_obj)); which do many things at the same line:

Call PyDict_GetItemWithError()
Call Py_XNewRef()
Assign current_param

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

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

Comment thread

Modules/_sqlite/statement.c Resolved

Uh oh!

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


          Address review comment: Split nested calls

8528bf6

erlend-aasland commented Nov 6, 2020

Copy link

Copy Markdown

Contributor Author

But I'm not sure about long lines like current_param = Py_XNewRef(PyDict_GetItemWithError(parameters, binding_name_obj)); which do many things at the same line:

Fixed in 8528bf6

All reactions

Sorry, something went wrong.

Uh oh!

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

Jongy commented Nov 8, 2020

Copy link

Copy Markdown

Contributor

Just created https://bugs.python.org/issue42287, then saw this PR, good work :) This Py_INCREF(x); y = x; hassle always annoyed me in CPython's sources.

I believe that such changes, when applied correctly (that is, if you did not change code semantics), should result in an identical binary. So a simple check to ensure you haven't broken anything would be to compare the final build before & after.

P.S make sure to strip debug info, because it does change (line numbers, ...)

@vstinner I think this PR (and future PRs regarding this conversion) do not require a news entry, right? because they do not introduce any user-facing change, only code enhancement and refactoring.

All reactions

Sorry, something went wrong.

Uh oh!

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


          Address Victor's review: Restore INCREF/XSETREF in Connection.__init__

68e044f

erlend-aasland requested a review from vstinner

November 8, 2020 08:14

erlend-aasland commented Nov 20, 2020

Copy link

Copy Markdown

Contributor Author

@vstinner Would you like further changes, or are you fine with this PR as it stands?

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Nov 23, 2020

View reviewed changes

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

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


          Address review

dd7d4e7

erlend-aasland requested a review from vstinner

November 23, 2020 10:08

vstinner added the skip news label

Nov 23, 2020

vstinner approved these changes

Nov 23, 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.

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with hooray emoji

All reactions

🎉 1 reaction

bedevere-bot added awaiting merge and removed awaiting review labels

Nov 23, 2020

Erlend Egeberg Aasland added 2 commits

December 18, 2020 23:52


          Merge branch 'master' into bpo-1635741/sqlite-new-ref

29c93db


          Fix merge

b7b4b78

erlend-aasland commented Dec 26, 2020

Copy link

Copy Markdown

Contributor Author

@vstinner FYI, synced with master, ready for merge when you please.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner merged commit bf64d90 into python:master

Dec 27, 2020

bedevere-bot removed the awaiting merge label

Dec 27, 2020

erlend-aasland deleted the bpo-1635741/sqlite-new-ref branch

December 27, 2020 11:06

erlend-aasland mentioned this pull request

Dec 27, 2020

bpo-1635741: Fix ref. leak introduced by bf64d90 #23972

Merged

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

Mar 13, 2021


          bpo-1635741: sqlite3 uses Py_NewRef/Py_XNewRef (pythonGH-23170)

103cbfa

kylotan mannequin mentioned this pull request

Sep 19, 2022

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

Closed

erlend-aasland mentioned this pull request

Nov 14, 2022

Replace Py_INCREF()/Py_XINCREF() usage with Py_NewRef()/Py_XNewRef() #99300

Closed

Reviewers

vstinner

vstinner approved these changes

berkerpeksag

Awaiting requested review from berkerpeksag

+1 more reviewer

Jongy

Jongy left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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

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

Uh oh!

Uh oh!

erlend-aasland commented Nov 5, 2020

Uh oh!

vstinner commented Nov 6, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Nov 6, 2020

Uh oh!

Jongy commented Nov 8, 2020

Uh oh!

erlend-aasland commented Nov 20, 2020

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Dec 26, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

erlend-aasland commented Nov 5, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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