Skip to content
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

Make C modules Python 3 compatible #76

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
99 changes: 83 additions & 16 deletions spitfire/runtime/_baked.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ static PyObject *Skip_Filter_PyString = NULL;

// SanitizedPlaceholder Object
typedef struct {
#if PY_MAJOR_VERSION >= 3
PyUnicodeObject str;
#else
PyStringObject str;
#endif
} SanitizedPlaceholderObject;

// Forward declare the type object.
Expand All @@ -22,6 +26,17 @@ static PyTypeObject SanitizedPlaceholderType;
static PyObject *
mark_as_sanitized(PyObject *self, PyObject *value)
{
#if PY_MAJOR_VERSION >= 3
// If the value is not unicode, return it immediately.
if (!PyUnicode_CheckExact(value)) {
Py_INCREF(value);
return value;
}
PyObject *args = Py_BuildValue("(N)", value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment on why you are choosing N (and therefore not incrementing the reference count) instead of O?

PyObject *obj = PyObject_CallObject((PyObject *) &SanitizedPlaceholderType, args);
Py_DECREF(args);
return obj;
#else
// If the value is not a string, return it immediately.
if (!PyString_CheckExact(value)) {
Py_INCREF(value);
Expand All @@ -31,6 +46,9 @@ mark_as_sanitized(PyObject *self, PyObject *value)
Py_ssize_t size = PyString_GET_SIZE(value);
// Allocate space for the SP object.
PyObject *obj = PyType_GenericAlloc(&SanitizedPlaceholderType, size);
if (obj == NULL) {
return NULL;
}
SanitizedPlaceholderObject *sp_ptr = (SanitizedPlaceholderObject *)obj;
// Set the hash and state.
sp_ptr->str.ob_shash = ((PyStringObject *)value)->ob_shash;
Expand All @@ -39,6 +57,7 @@ mark_as_sanitized(PyObject *self, PyObject *value)
// instantiating a new string object.
memcpy(sp_ptr->str.ob_sval, PyString_AS_STRING(value), size+1);
return obj;
#endif
}

// Python function that checks if skip_filter is present on the function passed
Expand Down Expand Up @@ -71,28 +90,38 @@ runtime_mark_as_sanitized(PyObject *self, PyObject *args)
static PyObject *
sp_concat(PyObject *self, PyObject *other)
{
PyObject *tmpself;
#if PY_MAJOR_VERSION >= 3
tmpself = PyUnicode_Concat(self, other);
#else
// PyString_Concat requires an INCREF on self.
Py_INCREF(self);
PyString_Concat(&self, other);
tmpself = self;
#endif
if (Py_TYPE(other) != &SanitizedPlaceholderType) {
return self;
return tmpself;
}
// PyString_Concat copies self turning it back into a string. Calling
// mark_as_sanitized turns it back into a SanitizedPlaceholder.
return mark_as_sanitized(self, self);
return mark_as_sanitized(tmpself, tmpself);
}

// SanitizedPlaceholder method for mod (%). A SanitizedPlaceholder is returned
// when both values are SanitizedPlaceholders.
static PyObject *
sp_mod(PyObject *self, PyObject *other)
{
PyObject * val = PyString_Format(self, other);
#if PY_MAJOR_VERSION >= 3
PyObject *val = PyUnicode_Format(self, other);
#else
PyObject *val = PyString_Format(self, other);
#endif
if (Py_TYPE(other) != &SanitizedPlaceholderType) {
return val;
}
// If the other value was a SanitizedPlaceholder, we want to turn the string
// result from PyString_Format into a SanitizedPlaceholder. This currently
// result from Py*_Format into a SanitizedPlaceholder. This currently
// copies over the string which is not the most efficient way to do this.
return mark_as_sanitized(self, val);
}
Expand All @@ -112,14 +141,15 @@ static PyNumberMethods sp_as_number = {
0, /* nb_add */
0, /* nb_subtract */
0, /* nb_multiply */
#if PY_MAJOR_VERSION < 3
0, /* nb_divide */
#endif
sp_mod, /* nb_remainder */
};

// SanitizedPlaceholder Type.
static PyTypeObject SanitizedPlaceholderType = {
PyObject_HEAD_INIT(NULL)
0, /* ob_size */
PyVarObject_HEAD_INIT(NULL,0 )
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PyVarObject_HEAD_INIT(NULL,0 )
PyVarObject_HEAD_INIT(NULL, 0)

"baked.SanitizedPlaceholder", /* tp_name */
sizeof(SanitizedPlaceholderObject), /* tp_basicsize */
0, /* tp_itemsize */
Expand All @@ -139,8 +169,12 @@ static PyTypeObject SanitizedPlaceholderType = {
0, /* tp_setattro */
0, /* tp_as_buffer */
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE |
Py_TPFLAGS_CHECKTYPES |
Py_TPFLAGS_STRING_SUBCLASS, /* tp_flags */
#if PY_MAJOR_VERSION >= 3
Py_TPFLAGS_UNICODE_SUBCLASS, /* tp_flags */
#else
Py_TPFLAGS_CHECKTYPES |
Py_TPFLAGS_STRING_SUBCLASS, /* tp_flags */
#endif
0, /* tp_doc */
0, /* tp_traverse */
0, /* tp_clear */
Expand All @@ -151,7 +185,7 @@ static PyTypeObject SanitizedPlaceholderType = {
0, /* tp_methods */
0, /* tp_members */
0, /* tp_getset */
0, /* tp_base */
&PyUnicode_Type, /* tp_base */
Copy link
Contributor

@nicksay nicksay Aug 28, 2019

Choose a reason for hiding this comment

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

Since we're conditionally setting tp_base below, let's not set it here too

Suggested change
&PyUnicode_Type, /* tp_base */
0, /* tp_base */

0, /* tp_dict */
0, /* tp_descr_get */
0, /* tp_descr_set */
Expand All @@ -169,25 +203,58 @@ static struct PyMethodDef baked_functions[] = {
{NULL, NULL}
};

PyMODINIT_FUNC
init_baked(void)
#if PY_MAJOR_VERSION >= 3
static struct PyModuleDef moduledef = {
PyModuleDef_HEAD_INIT,
"_baked", /* m_name */
"Baked Module", /* m_doc */
-1, /* m_size */
baked_functions, /* m_methods */
NULL, /* m_reload */
NULL, /* m_traverse */
NULL, /* m_clear */
NULL, /* m_free */
};
#endif

PyObject* moduleinit(void)
{
PyObject *m;

#if PY_MAJOR_VERSION >= 3
Skip_Filter_PyString = PyUnicode_InternFromString("skip_filter");
// Set the base type and the constructor.
SanitizedPlaceholderType.tp_base = &PyUnicode_Type;
SanitizedPlaceholderType.tp_init = PyUnicode_Type.tp_init;
#else
Skip_Filter_PyString = PyString_InternFromString("skip_filter");

// Set the base type and the constructor.
SanitizedPlaceholderType.tp_base = &PyString_Type;
SanitizedPlaceholderType.tp_init = PyString_Type.tp_init;
if (PyType_Ready(&SanitizedPlaceholderType) < 0)
return;
#endif

if (PyType_Ready(&SanitizedPlaceholderType) < 0) {
return NULL;
}

// Add exported functions to the module.
#if PY_MAJOR_VERSION >= 3
m = PyModule_Create(&moduledef);
#else
m = Py_InitModule3("_baked", baked_functions, "Baked Module");
if (m == NULL)
return;
#endif
if (m == NULL) {
return NULL;
}

Py_INCREF(&SanitizedPlaceholderType);
PyModule_AddObject(m, "_SanitizedPlaceholder",
(PyObject *) &SanitizedPlaceholderType);
return m;
}

#if PY_MAJOR_VERSION < 3
PyMODINIT_FUNC init_baked(void) { (void)moduleinit(); }
#else
PyMODINIT_FUNC PyInit__baked(void) { return moduleinit(); }
#endif
55 changes: 43 additions & 12 deletions spitfire/runtime/_template.c
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ static struct PyMethodDef template_methods[] = {

// BaseSpitfireTemplate Type.
static PyTypeObject BaseSpitfireTemplateType = {
PyObject_HEAD_INIT(NULL)
0, /* ob_size */
PyVarObject_HEAD_INIT(NULL, 0)
"template.BaseSpitfireTemplate", /* tp_name */
sizeof(BaseSpitfireTemplateObject), /* tp_basicsize */
0, /* tp_itemsize */
Expand Down Expand Up @@ -121,36 +120,68 @@ static struct PyMethodDef module_methods[] = {
{NULL} // Sentinel
};

#if PY_MAJOR_VERSION >= 3
static struct PyModuleDef moduledef = {
PyModuleDef_HEAD_INIT,
"_template", /* m_name */
"Template Module", /* m_doc */
-1, /* m_size */
module_methods, /* m_methods */
NULL, /* m_reload */
NULL, /* m_traverse */
NULL, /* m_clear */
NULL, /* m_free */
};
#endif

PyMODINIT_FUNC
init_template(void)
PyObject* moduleinit(void)
{
// Set interned strings.
#if PY_MAJOR_VERSION >= 3
Skip_Filter_PyString = PyUnicode_InternFromString("skip_filter");
filter_function_name = PyUnicode_InternFromString("_filter_function");
#else
Skip_Filter_PyString = PyString_InternFromString("skip_filter");
filter_function_name = PyString_InternFromString("_filter_function");
#endif

// Get SanitizedPlaceholder from the baked module.
PyObject *baked_module = PyImport_ImportModule("spitfire.runtime.baked");
if (baked_module == NULL)
return;
if (baked_module == NULL) {
return NULL;
}
baked_SanitizedPlaceholder = (struct _typeobject *)
PyObject_GetAttrString(baked_module, "SanitizedPlaceholder");
Py_DECREF(baked_module);
if (baked_SanitizedPlaceholder == NULL)
return;
if (baked_SanitizedPlaceholder == NULL) {
return NULL;
}


// Setup module and class.
PyObject *m;
BaseSpitfireTemplateType.tp_new = PyType_GenericNew;
if (PyType_Ready(&BaseSpitfireTemplateType) < 0)
return;
if (PyType_Ready(&BaseSpitfireTemplateType) < 0) {
return NULL;
}

#if PY_MAJOR_VERSION >= 3
m = PyModule_Create(&moduledef);
#else
m = Py_InitModule3("_template", module_methods, "Template Module");
if (m == NULL)
return;
#endif
if (m == NULL) {
return NULL;
}

Py_INCREF(&BaseSpitfireTemplateType);
PyModule_AddObject(m, "BaseSpitfireTemplate",
(PyObject *)&BaseSpitfireTemplateType);
return m;
}

#if PY_MAJOR_VERSION < 3
PyMODINIT_FUNC init_template(void) { (void)moduleinit(); }
#else
PyMODINIT_FUNC PyInit__template(void) { return moduleinit(); }
#endif
33 changes: 28 additions & 5 deletions spitfire/runtime/_udn.c
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ udn_resolve_udn(PyObject *self, PyObject *args, PyObject *kargs)
return NULL;
}

if (!(PyUnicode_Check(name) || PyString_Check(name))) {
if (!(PyUnicode_Check(name) || PyBytes_Check(name))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to either include bytesobject.h for python2.6+ compatibility of PyBytes_Check or add a PY_MAJOR_VERSION check here like you do elsewhere

PyErr_SetString(PyExc_ValueError, "name must be string");
return NULL;
}
Expand Down Expand Up @@ -110,7 +110,7 @@ udn_resolve_from_search_list(PyObject *self, PyObject *args, PyObject *keywds)
return NULL;
}

if (!(PyUnicode_Check(name) || PyString_Check(name))) {
if (!(PyUnicode_Check(name) || PyBytes_Check(name))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to either include bytesobject.h for python2.6+ compatibility of PyBytes_Check or add a PY_MAJOR_VERSION check here like you do elsewhere

PyErr_SetString(PyExc_ValueError, "name must be string");
return NULL;
}
Expand Down Expand Up @@ -155,15 +155,31 @@ static struct PyMethodDef udn_methods[] = {
{NULL, NULL}
};

#if PY_MAJOR_VERSION >= 3
static struct PyModuleDef moduledef = {
PyModuleDef_HEAD_INIT,
"_udn", /* m_name */
"_udn module", /* m_doc */
-1, /* m_size */
udn_methods, /* m_methods */
NULL, /* m_reload */
NULL, /* m_traverse */
NULL, /* m_clear */
NULL, /* m_free */
};
#endif

/* Initialization function (import-time) */

DL_EXPORT(void)
init_udn(void)
static PyObject* moduleinit(void)
{
PyObject *m, *runtime_module;

m = Py_InitModule("_udn", udn_methods);
#if PY_MAJOR_VERSION >= 3
m = PyModule_Create(&moduledef);
#else
m = Py_InitModule3("_udn", udn_methods, "_udn module");
#endif

runtime_module = PyImport_ImportModule("spitfire.runtime");
PlaceholderError = PyObject_GetAttrString(
Expand All @@ -177,8 +193,15 @@ init_udn(void)

if (PyErr_Occurred())
Py_FatalError("Can't initialize module _udn");
return m;
}

#if PY_MAJOR_VERSION < 3
PyMODINIT_FUNC init_udn(void) { (void)moduleinit(); }
#else
PyMODINIT_FUNC PyInit__udn(void) { return moduleinit(); }
#endif

#ifdef __cplusplus
}
#endif