Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-42972: Track sqlite3 statement objects #26475

Merged
merged 5 commits into from Jun 1, 2021

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented May 31, 2021

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented May 31, 2021

cc. @vstinner

Modules/_sqlite/cursor.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 31, 2021

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.

By allocating and tracking creation of statement object in
pysqlite_statement_create(), the caller does not need to worry about GC
syncronization, and eliminates the possibility of getting a badly
created object. All related fault handling is moved to
pysqlite_statement_create().
Copy link
Member

@vstinner vstinner left a comment

LGTM, but I would like to see the minor issues being addressed first before merging your change.

Modules/_sqlite/statement.c Outdated Show resolved Hide resolved
Modules/_sqlite/statement.c Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland force-pushed the erlend-aasland:sqlite-track-stmt branch from ee00a0d to 6b1ea0c Jun 1, 2021
erlend-aasland and others added 2 commits Jun 1, 2021
Co-authored-by: Victor Stinner <vstinner@python.org>
@erlend-aasland erlend-aasland force-pushed the erlend-aasland:sqlite-track-stmt branch from 6b1ea0c to 4f03b79 Jun 1, 2021
Copy link
Member

@vstinner vstinner left a comment

LGTM.

Modules/_sqlite/statement.c Show resolved Hide resolved
@vstinner vstinner merged commit fffa0f9 into python:main Jun 1, 2021
11 checks passed
11 checks passed
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
Azure Pipelines PR #20210601.11 succeeded
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@bedevere-bot
bedevere/issue-number Issue number 42972 found
Details
@bedevere-bot
bedevere/news "skip news" label found
@vstinner
Copy link
Member

@vstinner vstinner commented Jun 1, 2021

Merged, thanks.

@erlend-aasland erlend-aasland deleted the erlend-aasland:sqlite-track-stmt branch Jun 1, 2021
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 1, 2021

Merged, thanks.

Thanks for reviewing, Victor & Pablo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants