Skip to content

Consolidates index checking in NSOrderedSet #1469

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 16, 2018
Merged

Consolidates index checking in NSOrderedSet #1469

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 26 additions & 22 deletions Foundation/NSOrderedSet.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ open class NSOrderedSet : NSObject, NSCopying, NSMutableCopying, NSSecureCoding,
guard aCoder.allowsKeyedCoding else {
preconditionFailure("Unkeyed coding is unsupported.")
}
for idx in 0..<self.count {
for idx in _indices {
aCoder.encode(__SwiftValue.store(self.object(at: idx)), forKey:"NS.object.\(idx)")
}
}
Expand All @@ -67,6 +67,7 @@ open class NSOrderedSet : NSObject, NSCopying, NSMutableCopying, NSSecureCoding,
}

open func object(at idx: Int) -> Any {
_validateSubscript(idx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return __SwiftValue.fetch(nonOptional: _orderedStorage[idx])
}

Expand Down Expand Up @@ -120,11 +121,21 @@ open class NSOrderedSet : NSObject, NSCopying, NSMutableCopying, NSSecureCoding,
if type(of: self) === NSOrderedSet.self || type(of: self) === NSMutableOrderedSet.self {
return _orderedStorage.map { __SwiftValue.fetch(nonOptional: $0) }
} else {
return (0..<count).map { idx in
return _indices.map { idx in
return self[idx]
}
}
}

/// The range of indices that are valid for subscripting the ordered set.
internal var _indices: Range<Int> {
return 0..<count
}

/// Checks that an index is valid for subscripting: 0 ≤ `index` < `count`.
internal func _validateSubscript(_ index: Int, file: StaticString = #file, line: UInt = #line) {
precondition(_indices.contains(index), "\(self): Index out of bounds", file: file, line: line)
}
}

extension NSOrderedSet : Sequence {
Expand All @@ -148,9 +159,6 @@ extension NSOrderedSet {
open func objects(at indexes: IndexSet) -> [Any] {
var entries = [Any]()
for idx in indexes {
guard idx < count && idx >= 0 else {
Copy link
Contributor Author

@philium philium Mar 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This check is now performed by the call to object(at:) on line 162.

fatalError("\(self): Index out of bounds")
}
entries.append(object(at: idx))
}
return entries
Expand All @@ -177,7 +185,7 @@ extension NSOrderedSet {
return false
}

for idx in 0..<count {
for idx in _indices {
if let value1 = object(at: idx) as? AnyHashable,
let value2 = other.object(at: idx) as? AnyHashable {
if value1 != value2 {
Expand Down Expand Up @@ -338,9 +346,7 @@ extension NSOrderedSet {
open class NSMutableOrderedSet : NSOrderedSet {

open func insert(_ object: Any, at idx: Int) {
guard idx <= count && idx >= 0 else {
fatalError("\(self): Index out of bounds")
}
precondition(idx <= count && idx >= 0, "\(self): Index out of bounds")

let value = __SwiftValue.store(object)

Expand All @@ -353,15 +359,12 @@ open class NSMutableOrderedSet : NSOrderedSet {
}

open func removeObject(at idx: Int) {
_validateSubscript(idx)
_storage.remove(_orderedStorage[idx])
_orderedStorage.remove(at: idx)
}

open func replaceObject(at idx: Int, with obj: Any) {
guard idx < count && idx >= 0 else {
Copy link
Contributor Author

@philium philium Mar 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This check is now performed by the call to object(at:) on line 369.

fatalError("\(self): Index out of bounds")
}

let value = __SwiftValue.store(obj)
let objectToReplace = __SwiftValue.store(object(at: idx))
_orderedStorage[idx] = value
Expand Down Expand Up @@ -420,10 +423,6 @@ extension NSMutableOrderedSet {
}

open func exchangeObject(at idx1: Int, withObjectAt idx2: Int) {
guard idx1 < count && idx1 >= 0 && idx2 < count && idx2 >= 0 else {
Copy link
Contributor Author

@philium philium Mar 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

These checks are now performed by the calls to object(at:) on lines 426 and 427.

fatalError("\(self): Index out of bounds")
}

let object1 = self.object(at: idx1)
let object2 = self.object(at: idx2)
_orderedStorage[idx1] = __SwiftValue.store(object2)
Expand All @@ -450,13 +449,18 @@ extension NSMutableOrderedSet {
}
}

/// Sets the object at the specified index of the mutable ordered set.
///
/// - Parameters:
/// - obj: The object to be set.
/// - idx: The index. If the index is equal to `count`, then it appends
/// the object. Otherwise it replaces the object at the index with the
/// given object.
open func setObject(_ obj: Any, at idx: Int) {
let object = __SwiftValue.store(obj)
_storage.insert(object)
if idx == _orderedStorage.count {
_orderedStorage.append(object)
if idx == count {
insert(obj, at: idx)
} else {
_orderedStorage[idx] = object
replaceObject(at: idx, with: obj)
}
}

Expand Down