FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-1635741: sqlite3 now uses Py_NewRef and Py_XNewRef by erlend-aasland · Pull Request #23170 · python/cpython · GitHub

/ cpython Public

bpo-1635741: sqlite3 now uses Py_NewRef and Py_XNewRef - #23170

Merged
vstinner merged 6 commits into
python:masterfrom
erlend-aasland:bpo-1635741/sqlite-new-ref
Dec 27, 2020
ConversationCommits (6)ChecksFiles changed
Merged

bpo-1635741: sqlite3 now uses Py_NewRef and Py_XNewRef#23170
vstinner merged 6 commits into
python:masterfrom
erlend-aasland:bpo-1635741/sqlite-new-ref

Conversation

erlend-aasland commented Nov 5, 2020 •
edited by bedevere-bot
Loading

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

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?

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.

vstinner reviewed Nov 6, 2020
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Modules/_sqlite/connection.c
Comment thread Modules/_sqlite/statement.c Outdated

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

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.

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?

vstinner reviewed Nov 23, 2020
View reviewed changes
Comment thread Modules/_sqlite/connection.c
vstinner approved these changes Nov 23, 2020
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

erlend-aasland commented Dec 26, 2020

Copy link
Copy Markdown
Contributor Author

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

vstinner merged commit bf64d90 into python:master 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
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
Closed
erlend-aasland mentioned this pull request Nov 14, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL