Message115396
Thank you for the patch.
The patch doesn't handle the case where the object being searched for is !PyLong_CheckExact. For example, the user might pass in a sub-type of int. The existing range_contains supports that case, so it seems like we should support it in range_index and range_count.
The Sequence ABC's .index method doesn't include the optional start and stop parameters that are present on list.index. Since it's not part of the ABC, it's not mandatory that we implement those for range.index. The patch includes support for start and stop.
Attached is a greatly revised patch, with two significant changes:
- Adds support for !PyLong_CheckExact (using _PySequence_IterSearch)
- Drops support for the start and stop parameters to index
Dropping support for the start and stop parameters greatly simplified the code. If we want to support start and stop, then the code will need to get more complicated to handle the !PyLong_CheckExact case (since PySequence_IterSearch doesn't support start and stop).
My patch abstracts out most of the code that had originally been in range_contains into a new function range_contains_long so that it can be called by range_contains, range_count, and range_index. The diff makes that part look like a large change, but it's mostly a whitespace change (the refactored code lost one indentation level but is otherwise unchanged).
I uploaded Daniel Urban's patch and mine to Rietveld:
http://codereview.appspot.com/2146041/
Any strong feelings on whether range.index should support the start and stop arguments provided by list.index, tuple.index, and str.index, but not by collections.Sequence.index? |
|
Date |
User |
Action |
Args |
2010-09-02 19:18:04 | stutzbach | set | recipients:
+ stutzbach, rhettinger, mark.dickinson, eric.araujo, daniel.urban |
2010-09-02 19:18:04 | stutzbach | set | messageid: <1283455084.5.0.620679456196.issue9213@psf.upfronthosting.co.za> |
2010-09-02 19:18:02 | stutzbach | link | issue9213 messages |
2010-09-02 19:18:02 | stutzbach | create | |
|