Implement copying, indexing and enumeration for NSOrderedSet using NSMutableArray/NSMutableSet #1367

amorde · 2017-12-09T07:55:26Z

This is a follow up to #385

Were this to be merged, the only things left unimplemented would be description(withLocale:) and description(withLocale:indent:), excluding the sort options that are not yet implemented by NSArray

spevans · 2017-12-09T09:11:32Z

Foundation/NSOrderedSet.swift

    }

    public convenience override init() {
        self.init(objects: [], count: 0)
    }

    public init(objects: UnsafePointer<AnyObject>!, count cnt: Int) {
-        _storage = Set<NSObject>()


According to https://developer.apple.com/documentation/foundation/nsorderedset the two init functions that take an UnsafePointer are:
init(objects: UnsafePointer<AnyObject>?, count: Int) and
init(objects: UnsafePointer<AnyObject>, count: Int). This one needs to be fixed to match one of those, and also could you possibly add the other one?

Absolutely - good catch

spevans · 2017-12-09T09:13:07Z

@swift-ci please test

amorde · 2017-12-09T09:44:11Z

Foundation/NSOrderedSet.swift

-    public init(objects: UnsafePointer<AnyObject>!, count cnt: Int) {
-        _storage = Set<NSObject>()
-        _orderedStorage = [NSObject]()
+    public convenience init(objects: UnsafePointer<AnyObject>, count cnt: Int) {


Inspecting the init in Xcode showed this specific variant as a convenience init while the one below was not, so I matched the behavior here as well

If you follow the link for any of the methods in https://developer.apple.com/documentation/foundation/nsorderedset you will see the full function signature. It would be a good idea to do a quick check of all of the methods and bring them into line with the Foundation documentation.

Yep, I was checking this as I implemented them but missed this one. I will check again

Found a couple more that need to be optional instead of implicitly unwrapped

spevans · 2017-12-09T10:56:50Z

From https://ci.swift.org/view/Pull%20Request/job/swift-corelibs-foundation-PR-Linux/486/console it looks like there was a test failure:

TestFoundation/TestNSOrderedSet.swift:177: error: TestNSOrderedSet.test_RemoveObject : XCTAssertEqual failed: ("3") is not equal to ("2") -

spevans · 2017-12-10T12:05:05Z

@swift-ci please test

parkera · 2017-12-11T20:40:14Z

Do we need any additional tests for this functionality?

michael-lehew

Cocoa collections maintainer here, overall this looks like a good step towards improving things -- a few comments inline.

michael-lehew · 2017-12-11T20:42:10Z

Foundation/NSOrderedSet.swift

-        NSUnimplemented()
+        let mutableSelf = NSMutableOrderedSet(capacity: count)
+        mutableSelf.addObjects(from: self.array)
+        return mutableSelf


this is sufficient, but doesn't take advantage of the fact that we already know the elements are a set, so everything re-hashes, etc.

I looked at how NSArray is implemented and it copies over the _storage to the next object - should I use a similar mechanism here?

arrays don't get to make extra assumptions about their contents -- my point here was that the its possible to avoid the set-membership check (the set will still have to hash etc to maintain correct layout) but since we're already a runtime set, we already have satisfied uniqueness. its probably a small win -- but for common currency types, small wins matter a bit more.

michael-lehew · 2017-12-11T20:46:13Z

Foundation/NSOrderedSet.swift

            idx += 1
        }
-        self.init(array: objects)


the previous implementation is a little bit more correct I think. You don't want to have multiple initializers for coding. Coding should use the regular initializer as much as possible -- philosophically: in doing so its easier to maintain the run time invariants of your object, since they're automatically enforced.

Also from a correctness perspective -- its possible to construct a payload that will not actually come from a valid runtime ordered set (e.g. contain duplicates) and this does not detect such a malformed archive. Best approach there would be just create the ordered set and ask if its count == count of the array, returning nil if not equal.

Thank you for the insight, I'll take another look and see if I can get it closer to was it was before

michael-lehew · 2017-12-11T20:47:31Z

Foundation/NSOrderedSet.swift

-    internal var _storage: Set<NSObject>
-    internal var _orderedStorage: [NSObject]
+    internal var _storage: NSMutableSet
+    internal var _orderedStorage: NSMutableArray


this is essentially how the ObjC-based Darwin implementations are handled, so thats "A Nice Thing™"

Actually looking more closely -- I realize this is the NSOrderedSet impl -- why are these mutable?

Good catch - these no longer need to be vars

Although it does allow internal implementation to copy the _storage from one set to another, like when implementing copy

Sorry, I see I misunderstood your question. I could override these in NSMutableOrderedSet to be mutable and keep these ones as their non-mutable counter parts, but keeping these as mutable makes it easier to build the set incrementally in initializers

Building incrementally isn't the best for performance (sans capacity hints that are obeyed) - since you'll hit the internal vector growth storage of array/set many times.

michael-lehew · 2017-12-11T20:54:20Z

Foundation/NSOrderedSet.swift

    }

    open func union(_ other: NSOrderedSet) {
        other.forEach(add)
    }

    open func intersectSet(_ other: Set<AnyHashable>) {
-        for case let item as AnyHashable in self where !other.contains(item) {
+        for case let item as AnyHashable in self.reversed() where !other.contains(item) {


i assume you're doing this for performance reasons? there are still O(k) 'shifts' of the array to fill the holes from the removals. if you're going for clarity i'd remove the .reversed() if you're going for clarity combine the shift with a single O(n) pass. Something like:

// pseudo code i = 0 while i < count{ nextIndex = i + 1 if !other.contains(_ordereStorage[i]) { if (i+1 < count) { _orderedStorage[i] = _orderedStorage[i + 1] } else { _orderedStorage.removeLastObject() } // remove from the set too } else { i = i + 1 // move onto the next index } }

Doing it without reversed() resulted in an index out of bounds error, my guess was because the enumerator was not keeping track of mutations to the array. I could definitely take some time to rewrite this to improve performance

Here's a similar method in NSMutableArray for reference:

open func removeObjects(in otherArray: [Any]) { let set = Set(otherArray.map { $0 as! AnyHashable } ) for idx in (0..<count).reversed() { if let value = object(at: idx) as? AnyHashable { if set.contains(value) { removeObject(at: idx) } } } }

Maybe that could be improved a bit as well

I think I agree that array could be improved as well. :) something for the future

I have updated this to use an implementation that is based on your pseudocode - I think it could definitely be adapted to NSArray too!

michael-lehew · 2017-12-11T20:55:20Z

Foundation/NSPredicate.swift

@@ -196,9 +196,9 @@ extension NSMutableSet {

 extension NSOrderedSet {
    open func filtered(using predicate: NSPredicate) -> NSOrderedSet {
-        return NSOrderedSet(array: self.allObjects.filter({ object in
+        return NSOrderedSet(array: self.array.filter { object in


why go through the snapshot (and thus bridged) array here?

I took this portion from the other PR - I'll take another look for better options

I can't see a way around going through bridging, since the initializers to NSOrderedSet accept [Any]

I may be misunderstanding this though. Do you have any suggestions on a better way to do this?

I was thinking just using the ivar -- but I see that this in an extension and perhaps doesn't have access. The self.array will force a bridge from the NS type to the swift type, and so there's an extra semantic copy here (its perhaps not as bad as on Darwin platforms where we're crossing the objc->swift divide).

I added back the allObjects accessor, so this change is no longer present

amorde · 2017-12-13T01:51:43Z

I will clean up the commits a bit once the implementation is finalized

spevans · 2017-12-13T07:49:25Z

@swift-ci please test

millenomi · 2017-12-13T22:12:45Z

@swift-ci please test.

…MutableArray and NSMutableSet

amorde · 2018-01-17T22:40:30Z

@michael-lehew Do you have any other comments or suggestions for this? Since the discussion is growing quite large I am considering starting from scratch with a new PR, unless you feel this isn't the right direction to go at all

alblue · 2018-01-25T11:19:46Z

@swift-ci please test

spevans reviewed Dec 9, 2017

View reviewed changes

amorde commented Dec 9, 2017

View reviewed changes

michael-lehew reviewed Dec 11, 2017

View reviewed changes

Implement copying, indexing and enumeration for NSOrderedSet using NS…

1f409e1

…MutableArray and NSMutableSet

amorde closed this Mar 15, 2018

amorde mentioned this pull request Jan 6, 2019

Use NSArray/NSMutableArray and NSSet/NSMutableSet in NSOrderedSet #1816

Merged

Implement copying, indexing and enumeration for NSOrderedSet using NSMutableArray/NSMutableSet #1367

Implement copying, indexing and enumeration for NSOrderedSet using NSMutableArray/NSMutableSet #1367

Uh oh!

Conversation

amorde commented Dec 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spevans commented Dec 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spevans commented Dec 9, 2017

Uh oh!

spevans commented Dec 10, 2017

Uh oh!

parkera commented Dec 11, 2017

Uh oh!

michael-lehew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amorde Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amorde commented Dec 13, 2017

Uh oh!

spevans commented Dec 13, 2017

Uh oh!

millenomi commented Dec 13, 2017

Uh oh!

amorde commented Jan 17, 2018

amorde Dec 13, 2017 •

edited

Loading