Consolidates index checking in NSOrderedSet #1469
Conversation
| @@ -148,9 +164,6 @@ extension NSOrderedSet { | |||
| open func objects(at indexes: IndexSet) -> [Any] { | |||
| var entries = [Any]() | |||
| for idx in indexes { | |||
| guard idx < count && idx >= 0 else { | |||
There was a problem hiding this comment.
This check is now performed by the call to object(at:) on line 162.
| _storage.remove(_orderedStorage[idx]) | ||
| _orderedStorage.remove(at: idx) | ||
| } | ||
|
|
||
| open func replaceObject(at idx: Int, with obj: Any) { | ||
| guard idx < count && idx >= 0 else { |
There was a problem hiding this comment.
This check is now performed by the call to object(at:) on line 369.
| @@ -420,10 +428,6 @@ extension NSMutableOrderedSet { | |||
| } | |||
|
|
|||
| open func exchangeObject(at idx1: Int, withObjectAt idx2: Int) { | |||
| guard idx1 < count && idx1 >= 0 && idx2 < count && idx2 >= 0 else { | |||
There was a problem hiding this comment.
These checks are now performed by the calls to object(at:) on lines 426 and 427.
|
@swift-ci please test |
| @@ -67,6 +67,7 @@ open class NSOrderedSet : NSObject, NSCopying, NSMutableCopying, NSSecureCoding, | |||
| } | |||
|
|
|||
| open func object(at idx: Int) -> Any { | |||
| _validateSubscript(idx) | |||
There was a problem hiding this comment.
My thought is that there should be a check here since this is a public entry point. I presume that these sorts of checks are so that the failure happens at the current level (Foundation) rather than at a deeper level (the standard library).
There was a problem hiding this comment.
And by adding a check here, it made checks in other functions superfluous. I thought it better to prevent double-checking than to check ASAP at each public entry point.
Foundation/NSOrderedSet.swift
Outdated
|
|
||
| /// Checks that an index is valid: 0 ≤ `index` ≤ `count`. | ||
| internal func _validateIndex(_ index: Int, file: StaticString = #file, line: UInt = #line) { | ||
| precondition(index <= count && index >= 0, "\(self): Index out of bounds", file: file, line: line) |
There was a problem hiding this comment.
Should this be index < count?
There was a problem hiding this comment.
Oh I see, it's used for insertion as well as removal. Those are two different scenarios I think.
There was a problem hiding this comment.
Correct. They are two different scenarios, which is why there are two different validation methods: _validateIndex() (which includes count as a valid index) and _validateSubscript(). This method (_validateIndex()) is only used for insertObject() and setObject(), both of which accept count as a valid index.
There was a problem hiding this comment.
It was the names that were confusing me; validate index sounds like it must be a valid current index, but really it's a valid current index or "future" index.
Perhaps _validatePossibleIndex and _validateCurrentIndex; the latter could be used for more than just subscripting.
There was a problem hiding this comment.
That is an interesting idea. I guess a good question to ask is this: what is an index? I was borrowing from Swift's concept of a collection where endIndex is considered an index, even though it is not valid for subscripting. The indices property of Collection even documents that the returned indexes are only those that are valid for subscripting, implying that there are other indexes that are not valid for subscripting, namely endIndex. If count is an index, then _validateIndex() seems like a reasonable name for the method that validates count.
As you pointed out, whether an index is valid comes down to which scenario or context. The method insert(_:at:) has a parameter idx that will take count/endIndex as a valid value, meaning that count is a valid index in that context. On the other hand, the method object(at:) does not take count as an index, implying that count is not a valid index in that context. What's the difference? The second method is expecting an index that has a direct mapping to an element in the collection. The Swift documentation seems to favor the term subscript for that purpose.
From the perspective of either validate method, the index passed to it is a potential or "possible" index, so I'm not sure that _validatePossibleIndex would be any clearer.
Having said all that, I just had a different idea. The setObject(_:at:) method could be refactored as follows:
open func setObject(_ obj: Any, at idx: Int) {
if idx == count {
insert(obj, at: idx)
} else {
replaceObject(obj, at: idx)
}
}This essentially does what the Foundation documentation says it does:
If the index is equal to the length of the collection, then it inserts the object at that index, otherwise it replaces the object at that index with the given object.
In that case, the contents of _validateIndex() could just be moved into insertObject(_:at:) because that is the only place it would be used. If I were to do that, would you be satisfied with the existing name for the other validate method?
|
@philium Are you still interested in chaperoning this patch? |
|
@millenomi Yes. I've brought the branch up-to-date and made the changes I proposed. Please let me know what you think. |
|
@swift-ci please test and merge |
|
@millenomi It looks like the build failed due to a compilation error, but the error seems unrelated to my changes. Do you know what may have caused it? Could you please try again? |
|
@swift-ci test |
|
@philium Looks like it failed again. I would suggest rebasing the branch by doing: then force pushing it back to the PR |
Consolidates several index checks into object(at:) and adds a few index checks where it makes sense.
philium
force-pushed
the
consolidate-index-checking-in-NSOrderedSet
branch
from
f5a188b to
a72e793
Compare
|
@spevans Thank you. I've rebased my branch and force pushed the changes. Please try again. |
|
@swift-ci test |
|
@swift-ci please test and merge |
|
It looks like the last build succeeded, but the status hasn't been reported. |
|
@swift-ci please test and merge |
Consolidates several index checks into object(at:) and adds a few index
checks where it makes sense.