Build Parsing.py in the limited api #5845
Conversation
It isn't actually usable because it cimports other modules but it does build without warning. Changes are mainly to string handling (because that's the nature of parsing). Depends on cython#5841 and cython#5798 to actually build, but the PR itself doesn't depend on those
da-woods
force-pushed
the
limited-api-parsing
branch
from
771c50b
to
c7fe812
Compare
This PR (on top of cython#5845 and its dependencies) is sufficient to compile all the built modules in the Plex folder of Cython. It's currently unknown if they actually work (since it really needs all of Cython to be built to test).
Cython/Compiler/ExprNodes.py
Outdated
| @@ -14193,6 +14194,9 @@ def generate_result_code(self, code): | |||
| checks = ["(%s != Py_None)" % self.arg.py_result()] if self.arg.may_be_none() else [] | |||
| checks.append("(%s(%s) != 0)" % (test_func, self.arg.py_result())) | |||
| code.putln("%s = %s;" % (self.result(), '&&'.join(checks))) | |||
| code.putln("#if !CYTHON_ASSUME_SAFE_MACROS") | |||
| code.putln(code.error_goto_if_neg(self.result(), self.pos)) | |||
There was a problem hiding this comment.
This is unfortunate, because it may lead to an unused label. In rare cases, admittedly…
There was a problem hiding this comment.
I can rewrite it as
if ((!CYTHON_ASSUME_SAFE_MACROS) && %result% < 0) goto error
so the label is never unused. That might be slightly better (provided we manage to avoid "condition always true" warnings)
Cython/Utility/ObjectHandling.c
Outdated
| #if CYTHON_ASSUME_SAFE_MACROS | ||
| #define __Pyx_unpack_tuple2(tuple, value1, value2, is_tuple, has_known_size, decref_tuple) \ | ||
| (likely(is_tuple || PyTuple_Check(tuple)) ? \ | ||
| (likely(has_known_size || PyTuple_GET_SIZE(tuple) == 2) ? \ | ||
| __Pyx_unpack_tuple2_exact(tuple, value1, value2, decref_tuple) : \ | ||
| (__Pyx_UnpackTupleError(tuple, 2), -1)) : \ | ||
| __Pyx_unpack_tuple2_generic(tuple, value1, value2, has_known_size, decref_tuple)) | ||
| #else | ||
| static CYTHON_INLINE int __Pyx_unpack_tuple2( | ||
| PyObject* tuple, PyObject** value1, PyObject** value2, int is_tuple, int has_known_size, int decref_tuple); | ||
| #endif |
There was a problem hiding this comment.
It seems worth generally unfolding this lengthy macro into the inline function you wrote, and just special casing the size check.
Cython/Utility/StringTools.c
Outdated
| #if CYTHON_COMPILING_IN_LIMITED_API | ||
| // Note that from Python 3.7, the indices of FindChar account for wraparound so no | ||
| // need to check the length | ||
| Py_ssize_t idx = PyUnicode_FindChar(unicode, character, 0, -1, 1); |
There was a problem hiding this comment.
You could also pass PY_SSIZE_T_MAX as end to avoid the risk of an off-by-one error for -1 (because, is that inclusive or exclusive?).
Given that there's a C-API function for this, though, I wonder if we're really still faster than that. The implementation seems quite efficient. From my expecience, PyUnicode_READ() isn't the fastest thing to use, compared to a straight memory loop.
There was a problem hiding this comment.
I've taken this to mean "just use PyUnicode_FindChar in all cases". Happy to revert if this wasn't what you mean though!
Co-authored-by: scoder <stefan_ml@behnel.de>
Cython/Utility/StringTools.c
Outdated
| static CYTHON_INLINE PyObject* __Pyx_PyUnicode_Substring( | ||
| PyObject* text, Py_ssize_t start, Py_ssize_t stop); | ||
| #else | ||
| // In the limited API since 3.7 | ||
| #define __Pyx_PyUnicode_Substring(text, start, stop) PyUnicode_Substring(text, start, stop) |
There was a problem hiding this comment.
This isn't quite right - the __Pyx_PyUnicode_Substring function handles negative indices and PyUnicode_Substring doesn't. Will fix
This PR (on top of #5845 and its dependencies) is sufficient to compile all the built modules in the Plex folder of Cython.
Cython/Utility/Optimize.c
Outdated
| PyTuple_SET_ITEM(tuple, 0, key); | ||
| PyTuple_SET_ITEM(tuple, 1, value); | ||
| #else | ||
| if (unlikely(PyTuple_SetItem(tuple, 0, key) < 0)) { | ||
| Py_DECREF(value); // we haven't set this yet |
There was a problem hiding this comment.
We haven't actually set key in this case either. We need to decref both (and value in the failure case below).
There was a problem hiding this comment.
PyTuple_SetItem decrefs key on failure (https://github.com/python/cpython/blob/939fc6d6eab9b7ea8c244d513610dbdd556503a7/Objects/tupleobject.c#L122C10-L122C10) so that it effectively steals a reference whether or not it succeeds.
I'll add a comment because it isn't obvious though
Co-authored-by: scoder <stefan_ml@behnel.de>
|
I've cleaned up the |
|
Nice! |
It isn't actually usable because it cimports other modules but it does build without warning. Changes are mainly to string handling (because that's the nature of parsing).
Depends on #5841 and #5798 to actually build Parsing.py, but the PR itself can be applied independently.