-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
REF: use datetime C API instead of getattrs #51368
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea think we need to approach the implementation differently though
@@ -79,7 +79,9 @@ char *PyDateTimeToIso(PyObject *obj, NPY_DATETIMEUNIT base, | |||
npy_datetimestruct dts; | |||
int ret; | |||
|
|||
ret = convert_pydatetime_to_datetimestruct(obj, &dts); | |||
PyDateTime_IMPORT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the cpython source the datetime library seems to be implemented as a capsule. There's some documentaiton on that here:
https://docs.python.org/3/extending/extending.html#providing-a-c-api-for-an-extension-module
I really don't want to keep scattering these imports throughout the code base because it goes against the CPython documentation - can you check if the capsule approach would work?
ret = convert_pydatetime_to_datetimestruct(obj, &dts); | ||
PyDateTime_IMPORT; | ||
|
||
ret = convert_pydatetime_to_datetimestruct((PyDateTime_Date*)obj, &dts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to avoid blind casting - these should all be preceded by a PyDate_Check
call with appropriate error handling, otherwise its anybody guess what happens when you pass the wrong object type to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so something like
PyDateTime_IMPORT;
if (!PyDate_Check(obj)) {
PyErr_SetString(PyExc_ValueError,
"Object is not a datetime.date object");
return NULL;
}
PyDateTime_Date *dtobj = (PyDateTime_Date*)obj;
ret = convert_pydatetime_to_datetimestruct(dtobj, &dts);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in objToJSON.c L1552 we call PyDateTimeToEpoch but IIUC obj
ther eis a np.datetime64 object, not a pydatetime object. Isn't that call doomed to failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea you have got the right idea with that error handling - nice job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is the case I think that behavior would be undefined, so could segfault or get garbage values. Do you know of a reproducer?
if (PyObject_HasAttrString(obj, "tzinfo")) { | ||
PyObject *offset = extract_utc_offset(obj); | ||
// TODO(py3.10): in py3.10 we can use PyDateTime_DATE_GET_TZINFO | ||
PyObject *offset = extract_utc_offset((PyObject*)obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this cast necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without it i see a bunch of warnings
warning: incompatible pointer types passing 'PyDateTime_DateTime *' to parameter of type 'PyObject *' (aka 'struct _object *') [-Wincompatible-pointer-types]
PyObject *offset = extract_utc_offset(obj);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha makes sense
it works locally, but im seeing build failures from the type checking |
Closing as not worth the trouble. |
@WillAyd i think there are a couple of paths that must not be reached, since it looks like they pass np.datetime64 objects to these pydatetime-requiring functions. in python/cython id check that with an
assert False
. have a favorite way of doing that in c code?