GeezerVison

For the rationale of adding PySys_AuditTuple() in addition to the existing PySys_Audit(), see the discussion on my PR #108571 "Add PySys_Audit() to the limited C API" and this discussion: capi-workgroup/problems#35 (comment)

In short, it's possible to use the Python C API in Go, but Go doesn't support ... variadic arguments (<stdarg.h>). I would like to add PySys_Audit() argument to the limited C API, but @encukou asked me to add a different flavor which accepts a tuple. So here you have.

cc @encukou

cc @zooba

Please don't make every PySys_Audit call convert the arguments to a tuple eagerly. It deliberately defers it until it knows there's at least one hook that's going to be called.

Please don't slow down the regular API for the sake of the limited API. If anything, make PySys_AuditTuple just pass its tuple to PySys_Audit, which will hide the ... from callers but otherwise not change any other code.

Please don't make every PySys_Audit call convert the arguments to a tuple eagerly. It deliberately defers it until it knows there's at least one hook that's going to be called.

Oh! Nicely spotted, I knew that, I recall that I paid attention to that when you implemented audit hooks :-) Oops, it's now fixed. I made sure that "should audit" is checked as soon as possible. In exchange, arguments are only checked if a audit hook is added (if "should audit" is true).

Please don't slow down the regular API for the sake of the limited API. If anything, make PySys_AuditTuple just pass its tuple to PySys_Audit, which will hide the ... from callers but otherwise not change any other code.

I fixed my PR, existing APIs are still lazy as before, and I made PySys_AuditTuple() a litlte bit more lazy (exit earlier, before checking arguments).

@zooba: Would you mind to review the updated PR?

Please don't slow down the regular API for the sake of the limited API.

Oh by the way, as I wrote earlier: I would prefer to not add PySys_AuditTuple() to the limited C API.

I can't do a thorough review of that much code right now.

Why isn't the new function just this?

int PySys_AuditTuple(const char *event, PyObject *tuple) {
    return PySys_Audit(event, "O", tuple);
}

If a single object is passed in and it's a tuple, it's meant to be left alone (i.e. splatted). If you want a single argument that happens to be a tuple, you need PySys_Audit(event, "(O)", tuple).

If a single object is passed in and it's a tuple, it's meant to be left alone (i.e. splatted).

Hum, I dislike this kind of magic "unpack or not" depending on the number of items. What if an event takes a single argument which a tuple? What if you want to unpack the tuple since the event has multiple arguments?

I would prefer a regular API: always pass a tuple.

The magic already exists, and I showed in my message how to handle the case where you pass a tuple. It's also in the docs. If you want to make a massively complicated change instead of using the already existing API, that's up to you, but it's going to make it much harder to get a review and will cause maintenance headaches for all of us later on.

I concur with @zooba.

Ok, I updated my PR so PySys_AuditTuple() simply calls PySys_Audit(). I updated the doc and the added tests.

@zooba @serhiy-storchaka: Would you mind to review the updated PR?

Ok, I updated my PR so PySys_AuditTuple() simply calls PySys_Audit().

Oh, I removed too many changes and new tests failed. I added again checks on event and format to fix tests.

Ok, I updated my PR to add assertions to check arguments when should_audit() is false, and I added a PyTuple_Check() to reject types other than tuple.

@serhiy-storchaka: Would you mind to review the updated PR?

Note: sys_audit() is implemented by calling _PySys_Audit(tstate, event, "O", auditArgs).

@zooba @serhiy-storchaka: I updated my PR to address latest @zooba's review, would you mind to review it?

• Assertions are now also tested in all cases, not only if should_audit() is false. Well, never caled these functions with invalid arguments!
• Fix the doc.
• Replace assert() with printf+return in test_audit_tuple.

I rebased my PR.

@serhiy-storchaka: I updated my PR to take your review in account.

SystemError is now raised if events is NULL or if the "N" format option is used. I documented the error, as requested by @zooba.

@zooba:

Overall the change is good, but I don't like having sometimes assertions and sometimes exceptions for invalid values in debug builds.
Also you should benchmark the extra runtime checks you're doing now. Previously I benchmarked and found that I needed to use assertions to maintain performance when adding these functions in the first place, so if you're going to regress them, it should be its own discussion and not slipped into an unrelated change.

Ok. I removed the two checks which raised exceptions to leave PySys_Audit() and sys.audit() unchanged. I only added assertions to sys.audit() to check arguments even if there is no hook. They can be added again separately later if needed.

Ok, I addressed all reviews. Thanks a lot for all your reviews :-) I merged my PR.

Thank you Victor.