bpo-34229: Check start and stop of slice object to be long when they are not int in PySlice_GetIndices by tirkarthi · Pull Request #8480 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

tirkarthi

Check for slice.start and slice.stop to be Long when they are not int in PySlice_GetIndices instead of checking for slice.step to be of Long type.

Thanks

https://bugs.python.org/issue34229

@tirkarthi

@tirkarthi tirkarthi changed the title[2.7] bpo-34229: Check start and stop of slice object to be long when they are not int in PySlice_GetIndices bpo-34229: Check start and stop of slice object to be long when they are not int in PySlice_GetIndices

Jul 26, 2018

serhiy-storchaka

if (!PyArg_ParseTuple(args, "On", &slice, &length))
return NULL;
int result = PySlice_GetIndices(slice, length, &start, &stop, &step);

Choose a reason for hiding this comment

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

Return NULL if an error is set.

Choose a reason for hiding this comment

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

The method PySlice_GetIndices only returns an int and I couldn't see it raising any error. I might be missing something on returning NULL based on error here. Is it about returning NULL based on result to be -1? I am still a beginner in C trying to improve so please bear with me on this point.

Choose a reason for hiding this comment

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

I think you meant the below code :

if (result == -1 && PyErr_Occurred())
    return NULL

Choose a reason for hiding this comment

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

Right. Actually result == -1 && is not needed, it is used only for performance, but in this case it doesn't matter. So you can write

if (PyErr_Occurred()) { assert(result == -1); return NULL; }

PyInt_AsSsize_t() in PySlice_GetIndices can raise an OverflowError.

return NULL;
int result = PySlice_GetIndices(slice, length, &start, &stop, &step);
return Py_BuildValue("i", result);

Choose a reason for hiding this comment

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

Is it worth to return also start, stop and step?

Choose a reason for hiding this comment

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

I tried this but since when r->start is a float then -1 is returned and *start is not set. *start points to value like 140737488324304 that was there during initialization which I can't compare in the test case. Any thoughts on this?

Choose a reason for hiding this comment

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

What if return a tuple only if result != -1?

if (result == -1) { PY_RETURN_NONE; } return Py_BuildValue("innn", result, r->start, r->stop, r->step);

Choose a reason for hiding this comment

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

I hope you meant return Py_BuildValue("innn", result, start, stop, step); since r->start is not changed inside the function and we only set *start which we can compare back in the test.

static PyObject *
get_indices(PyObject *self, PyObject *args)
{
PySliceObject *slice;

Choose a reason for hiding this comment

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

Please use 4-space indentation.

class TestGetIndices(unittest.TestCase):
def test_get_indices(self):
self.assertEqual(_testcapi.get_indices(slice(10L, 20, 1), 100), 0)

Choose a reason for hiding this comment

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

Repeat the same tests with step is long.

@tirkarthi

Unfortunately, I don't have a windows machine to debug the Appveyor failure. From the error message I guess it's due to not casting it with PyCFunction and {"get_indices", (PyCFunction)get_indices, METH_VARARGS} should fix the issue.

@tirkarthi

@tirkarthi

@tirkarthi

Appveyor still fails even with the casting. I guess it's due to something other windows compiler specific issue since Travis passes. Any pointers will be helpful.

Thanks

serhiy-storchaka

if (!PyArg_ParseTuple(args, "On", &slice, &length))
return NULL;
int result = PySlice_GetIndices(slice, length, &start, &stop, &step);

Choose a reason for hiding this comment

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

2.7 uses C89. All variables should be declared at the start of the block.

@tirkarthi

serhiy-storchaka

@tirkarthi

@serhiy-storchaka Thanks much for your review guidance on this. I have opened a ticket regarding test case failure in 2.7 in test_capi.py that I found while working on this.

Thanks