bpo-34925: Optimize common case for bisect() argument parsing#9753
bpo-34925: Optimize common case for bisect() argument parsing#9753rhettinger merged 4 commits intopython:masterfrom
Conversation
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Looks correct, besides few style issues.
I hope this case will be automatically optimized by Argument Clinic in 3.8.
| _Py_IDENTIFIER(insert); | ||
|
|
||
| static Py_ssize_t | ||
| static inline Py_ssize_t |
There was a problem hiding this comment.
Perhaps you meant Py_LOCAL_INLINE(Py_ssize_t)?
There was a problem hiding this comment.
Is that being used anymore? I believe we're using "inline" directly (and some other C99 features) nowadays.
There was a problem hiding this comment.
I don't know. Maybe Py_LOCAL_INLINE is outdated.
It is expanded to a different form on MSVC.
Modules/_bisectmodule.c
Outdated
| if (!PyArg_ParseTupleAndKeywords(args, kw, "OO|nn:bisect_right", | ||
| keywords, &list, &item, &lo, &hi)) | ||
| return NULL; | ||
| if (kw==NULL && PyTuple_GET_SIZE(args)==2) { |
There was a problem hiding this comment.
Sorry for be pedantic, but could you please add spaces around == for conforming PEP 7? Without spaces the code looks sloppy to me.
There was a problem hiding this comment.
IIRC, when multiple terms are present, PEP 7 (and PEP 8) allow for tightening spaces on the inner terms while "adding" spaces around the outermost (lowest-priority) operators.
There was a problem hiding this comment.
I think this is allowed only for arithmetic operators like * in +.
Always put spaces around assignment, Boolean and comparison operators. In expressions using a lot of operators, add spaces around the outermost (lowest-priority) operators.
Modules/_bisectmodule.c
Outdated
| if (kw==NULL && PyTuple_GET_SIZE(args)==2) { | ||
| list = PyTuple_GET_ITEM(args, 0); | ||
| item = PyTuple_GET_ITEM(args, 1); | ||
| } else { |
There was a problem hiding this comment.
For PEP 7 } and else { should be on separate lines.
}
else {There was a problem hiding this comment.
I see that in PEP 7 but it looks weird to me. I believe we historically allowed "} else {" in multiple places in the code when that reflects how the author thinks about the code. If you insist, I'll change it, but I like it better as-is.
There was a problem hiding this comment.
No, I do not insist. I added this comment just for the case if you missed this part of PEP 7 and do not have preferences. I always write this code as two lines in new and modified code, but writing it as a single line LGTM too.
The only thing looks weird to me is messed spaces around ==.
|
@rhettinger: Please replace |
4 participants
https://bugs.python.org/issue34925