Skip to content

Removed PyString refs from extension modules #28322

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.

Sign up for GitHub

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 2 commits into from
Sep 7, 2019
Merged

Removed PyString refs from extension modules #28322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
190479a
Removed PyString refs from extension modules
WillAyd Sep 6, 2019
a6eaf36
Reverted macro
WillAyd Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions pandas/_libs/src/parse_helper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ int to_double(char *item, double *p_value, char sci, char decimal,
return (error == 0) && (!*p_end);
}

#if PY_VERSION_HEX < 0x02060000
#define PyBytes_Check PyString_Check
#define PyBytes_AS_STRING PyString_AS_STRING
#endif // PY_VERSION_HEX

int floatify(PyObject *str, double *result, int *maybe_int) {
int status;
char *data;
Expand Down
4 changes: 2 additions & 2 deletions pandas/_libs/src/ujson/python/objToJSON.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ static void *PyFloatToDOUBLE(JSOBJ _obj, JSONTypeContext *tc, void *outValue,
return NULL;
}

static void *PyStringToUTF8(JSOBJ _obj, JSONTypeContext *tc, void *outValue,
static void *PyBytesToUTF8(JSOBJ _obj, JSONTypeContext *tc, void *outValue,
size_t *_outLen) {
PyObject *obj = (PyObject *)_obj;
*_outLen = PyBytes_GET_SIZE(obj);
Expand Down Expand Up @@ -1869,7 +1869,7 @@ void Object_beginTypeContext(JSOBJ _obj, JSONTypeContext *tc) {
return;
} else if (PyBytes_Check(obj)) {
PRINTMARK();
pc->PyTypeToJSON = PyStringToUTF8;
pc->PyTypeToJSON = PyBytesToUTF8;
tc->type = JT_UTF8;
return;
} else if (PyUnicode_Check(obj)) {
Expand Down
5 changes: 0 additions & 5 deletions pandas/_libs/tslibs/util.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@ from cpython cimport PyTypeObject
cdef extern from *:
"""
PyObject* char_to_string(const char* data) {
#if PY_VERSION_HEX >= 0x03000000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's one more PY_VERSION_HEX check in compat_helper that could go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I was looking at that but I'm planning on the compat stuff in a separate PR

return PyUnicode_FromString(data);
#else
return PyString_FromString(data);
#endif
}
"""
object char_to_string(const char* data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt L14 be removed too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually used here:

pandas/pandas/_libs/tslibs/period.pyx

Line 1267 in 2d65e38

result = util.char_to_string(formatted)

So maybe but would require a more comprehensive refactor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that just becomes result = PyUnicode_FromString(formatted)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool - I'll give it a look. Seems like this caused a somewhat surprising failure so that might solve

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm so didn't realize the cdef of the tripled quoted string was actually using that as a macro. Was thinking of cimporting PyUnicode_FromString directly but looks like that isn't available until Cython 3.0.0

cython/cython#3010

For now going to revert to macro

Expand All @@ -18,7 +14,6 @@ cdef extern from "Python.h":
# Note: importing extern-style allows us to declare these as nogil
# functions, whereas `from cpython cimport` does not.
bint PyUnicode_Check(object obj) nogil
bint PyString_Check(object obj) nogil
bint PyBool_Check(object obj) nogil
bint PyFloat_Check(object obj) nogil
bint PyComplex_Check(object obj) nogil
Expand Down
9 changes: 1 addition & 8 deletions pandas/_libs/writers.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@ from cython import Py_ssize_t

from cpython cimport PyBytes_GET_SIZE, PyUnicode_GET_SIZE

try:
from cpython cimport PyString_GET_SIZE
except ImportError:
from cpython cimport PyUnicode_GET_SIZE as PyString_GET_SIZE

import numpy as np
from numpy cimport ndarray, uint8_t

Expand Down Expand Up @@ -126,11 +121,9 @@ def max_len_string_array(pandas_string[:] arr) -> Py_ssize_t:
for i in range(length):
val = arr[i]
if isinstance(val, str):
l = PyString_GET_SIZE(val)
l = PyUnicode_GET_SIZE(val)
elif isinstance(val, bytes):
l = PyBytes_GET_SIZE(val)
elif isinstance(val, unicode):
l = PyUnicode_GET_SIZE(val)

if l > m:
m = l
Expand Down