Skip to content

bpo-36842: Implement PEP 578 #12613

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

Merged
merged 33 commits into from
May 23, 2019
Merged

bpo-36842: Implement PEP 578 #12613

merged 33 commits into from
May 23, 2019

Conversation

zooba
Copy link
Member

@zooba zooba commented Mar 28, 2019

@zooba zooba marked this pull request as ready for review April 10, 2019 18:48
@zooba zooba requested a review from a team April 10, 2019 18:48
@zooba
Copy link
Member Author

zooba commented Apr 10, 2019

The PEP is not approved yet, of course, so the PR isn't finished. But I want it to at least run through CI now to catch anything that's going on.

@zooba zooba closed this Apr 10, 2019
@zooba zooba reopened this Apr 10, 2019
@brettcannon brettcannon removed the request for review from a team April 17, 2019 22:46
@brettcannon
Copy link
Member

Removing the import team from reviewing as this is still a WIP. We can be added back when it's ready to go.

@zooba zooba changed the title Implement PEP 578 bpo-36842: Implement PEP 578 May 7, 2019
@zooba zooba requested a review from a team as a code owner May 8, 2019 15:29
@vstinner
Copy link
Member

Patch to use METH_FASTCALL:

diff --git a/Python/sysmodule.c b/Python/sysmodule.c
index f987bb2016..fd1408c748 100644
--- a/Python/sysmodule.c
+++ b/Python/sysmodule.c
@@ -22,6 +22,7 @@ Data members:
 #include "pycore_pymem.h"
 #include "pycore_pathconfig.h"
 #include "pycore_pystate.h"
+#include "pycore_tupleobject.h"
 #include "pythread.h"
 #include "pydtrace.h"
 
@@ -360,20 +361,14 @@ PyDoc_STRVAR(audit_doc,
 Passes the event to any audit hooks that are attached.");
 
 static PyObject *
-sys_audit(PyObject *self, PyObject *args)
+sys_audit(PyObject *self, PyObject *const *args, Py_ssize_t argc)
 {
-    if (!PyTuple_Check(args)) {
-        PyErr_Format(PyExc_TypeError, "expected tuple, not %.200s", Py_TYPE(args)->tp_name);
-        return NULL;
-    }
-
-    Py_ssize_t argc = PyTuple_GET_SIZE(args);
     if (argc == 0) {
         PyErr_SetString(PyExc_TypeError, "audit() missing 1 required positional argument: 'event'");
         return NULL;
     }
 
-    PyObject *auditEvent = PyTuple_GET_ITEM(args, 0);
+    PyObject *auditEvent = args[0];
     if (!auditEvent) {
         PyErr_SetString(PyExc_TypeError, "expected str for argument 'event'");
         return NULL;
@@ -388,16 +383,10 @@ sys_audit(PyObject *self, PyObject *args)
         return NULL;
     }
 
-    PyObject *auditArgs = PyTuple_New(argc - 1);
+    PyObject *auditArgs = _PyTuple_FromArray(args + 1, argc - 1);
     if (!auditArgs) {
         return NULL;
     }
-    for (int i = 0; i < argc - 1; ++i) {
-        PyObject *o = PyTuple_GET_ITEM(args, i + 1);
-        Py_INCREF(o);
-        PyTuple_SET_ITEM(auditArgs, i, o);
-    }
-
     int res = PySys_Audit(event, "O", auditArgs);
     Py_DECREF(auditArgs);
 
@@ -1924,7 +1913,7 @@ sys_getandroidapilevel_impl(PyObject *module)
 static PyMethodDef sys_methods[] = {
     /* Might as well keep this in alphabetic order */
     SYS_ADDAUDITHOOK_METHODDEF
-    {"audit",           sys_audit, METH_VARARGS, audit_doc },
+    {"audit",           (PyCFunction)(void(*)(void))sys_audit, METH_FASTCALL, audit_doc },
     {"breakpointhook",  (PyCFunction)(void(*)(void))sys_breakpointhook,
      METH_FASTCALL | METH_KEYWORDS, breakpointhook_doc},
     SYS_CALLSTATS_METHODDEF

By the way, I might be useful to extract the code from PySys_Audit() to decide if audit is used or not: create a subfunction, and call it from sys_audit() to do nothing if audit is not used (common case).

Calling PyUnicode_AsUTF8() and _PyTuple_FromArray() is not free :-) (I know that they are fast, but many function calls take less than 100 ns overall.)

@zooba
Copy link
Member Author

zooba commented May 15, 2019

@tiran I believe all feedback has been addressed, and all tests pass (custom buildbot run going now). Anything else?

@zooba
Copy link
Member Author

zooba commented May 17, 2019

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

@zooba
Copy link
Member Author

zooba commented May 21, 2019

@tiran I know you're busy with your PEP, but this is waiting on you.

I'll give it two more merge conflicts from other commits before I consider your feedback addressed and merge :)

@zooba
Copy link
Member Author

zooba commented May 23, 2019

That's the second conflict, so I'm dismissing @tiran's review and we can deal with any further issues post-commit.

@zooba zooba dismissed tiran’s stale review May 23, 2019 15:22

E_TIMEOUT

@zooba zooba merged commit b82e17e into python:master May 23, 2019
@zooba zooba deleted the pep-578 branch May 23, 2019 15:45
zooba added a commit to zooba/cpython that referenced this pull request Aug 21, 2019
Adds sys.audit, sys.addaudithook, io.open_code, and associated C APIs.
@zooba zooba mentioned this pull request Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants