Skip to content

bpo-40956: Use Argument Clinic in sqlite3 module #20826

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

Closed
wants to merge 9 commits into from

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 12, 2020

@erlend-aasland
Copy link
Contributor Author

There's a couple more ref leaks in this PR that I haven't been able to fix yet, @berkerpeksag. (I should have checked this before pushing, sorry 'bout that.) At least one of the ref leaks are related to isolation_level (connection.c). I'll let you know when I've pinned those down.

@erlend-aasland
Copy link
Contributor Author

I've updated the PR and fixed the following:

  • Only apply Argument Clinic (don't change parameter names, variable names, any other code)
  • Apply AC to all methods and function (even prepare_protocol.c)
  • Fixed all ref. leaks (Checked with ./python.exe -m test -F -r -j1 -R 3:10 test_sqlite)

Note 1: I've used _PyObject_CallMethodId* iso. pysqlite_cache_get() (fc89d4c853c498b8e51edd6787bdb8bcacd7279c) and pysqlite_connection_commit() (f646fdebfc810b30fc122922f0230e1b8b0d78c0), and iso. the cursor.execute* calls in connection.c (f646fdebfc810b30fc122922f0230e1b8b0d78c0). AFAICS, that should be an acceptable and clean solution.

Note 2: Converting _sqlite3.connect and _sqlite3.Connection.__init__ is not trivial. The "best" solution I could come up with (without recreating args and kwargs) was to use PyObject_CallFunctionObjArgs. However, then I need to ensure that none of the arguments are NULL, so I had to copy parts of the isolation_level code from connection.c. Very ugly. I'm sure there's a better way to do this, but I don't know the C API good enough to come up with a better solution right now. (One solution is of course to revert commit 0b456f1c08d879833be6c9d5b161bbc688defe60 ...)

PS. Sorry for force-pushing again. I'll try to avoid it.

@erlend-aasland
Copy link
Contributor Author

FYI: Rebased against master to resolve conflicts. Tests still pass without reference leaks.

@erlend-aasland erlend-aasland marked this pull request as draft October 1, 2020 15:39
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 1, 2020

Here, @vstinner. I'll rebase onto master and force push.

@vstinner
Copy link
Member

vstinner commented Oct 1, 2020

Sorry, I cannot review a PR which changes 19 files. Also, the PR has many conflicts.

@erlend-aasland
Copy link
Contributor Author

Sorry, I cannot review a PR which changes 19 files. Also, the PR has many conflicts.

Understandable! Let's repeat the previous exercise; I'll close this PR and split it up in manageable parts.

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

Successfully merging this pull request may close these issues.

4 participants