bpo-28754: AC on bisect. #177
Conversation
There was a problem hiding this comment.
LGTM.
But please wait approval from expert.
(@rhettinger is listed on http://cpython-devguide.readthedocs.io/experts.html)
| @@ -6,6 +6,39 @@ Converted to C by Dmitry Vasiliev (dima at hlabs.spb.ru). | |||
| #define PY_SSIZE_T_CLEAN | |||
| #include "Python.h" | |||
|
|
|||
| static int | |||
| ssize_t_converter(PyObject *obj, void *ptr) | |||
There was a problem hiding this comment.
ssize_t_converter is not used for parsing arguments. It is used only in optional_ssize_t_converter. Wouldn't be better to inline it?
|
|
||
| a: object | ||
| x: object | ||
| lo: Py_ssize_t(c_default='0') = 0 |
There was a problem hiding this comment.
c_default='0' is redundant.
|
|
||
|
|
||
| /*[clinic input] | ||
| bisect.bisect_right |
There was a problem hiding this comment.
You can use the return converter for simplifying the code.
bisect.bisect_right -> Py_ssize_t
The body of bisect_bisect_right_impl() would be just:
return internal_bisect_right(a, x, lo, hi);
| @@ -186,6 +186,11 @@ def test_optionalSlicing(self): | |||
| self.assertTrue(data[ip-1] <= elem) | |||
| self.assertEqual(ip, max(lo, min(hi, expected))) | |||
|
|
|||
| def test_hi_backcompatbility(self): | |||
| for func, data, elem, expected in self.precomputedCases: | |||
| self.assertEqual(func(data, elem, 0, -1), | |||
There was a problem hiding this comment.
This doesn't check that passing -1 and None is the same as not specifying the hi argument. Compare results with func(data, elem).
|
I don't want the pure python API to change at all. It is a long standing API and there is nothing wrong with it. Also, passing it -1 here as a special value makes real code less readable. Further, the change slightly slows down the case where high is specified. In a high performance building block module, we shouldn't add in unnecessary checks. |
|
To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request. If/when the requested changes have been made, please leave a comment that says, |
…ute() I missed the C-fame case, when I changed the frame-function ref counting in Stackless issue python#117.
[bpo-28754]