TL;DR: Remove the isUniquelyReferenced<T: NonObjectiveCBase>(_ object: T) API. All its current use cases can be satisfied equally performant by the isUniquelyReferencedNonObjC<T : AnyObject>(_ object: T) API.
This will reduce API surface by both the NonObjectiveCBase class and the isUniquelyReferenced API. I think this is pretty uncontroversial, too? [Today’s implementation of both APIs is the same builtin call. The compiler can optimize out the check for objective-c class ancestry when it sees that the static type of ‘object’ is not an @objc-class.] > On Jul 16, 2016, at 10:50 PM, Arnold Schwaighofer via swift-evolution > <[email protected]> wrote: > > Updated proposal: > > Remove NonObjectiveCBase and isUniquelyReferenced > > • Proposal: SE-0000 > • Author: Arnold Schwaighofer > • Status: Pitch > • Review manager: TBD > Introduction > > Remove NonObjectiveCBase and isUniquelyReferenced<T: NonObjectiveCBase>(_ > object: T). isUniquelyReferenced can be replaced by > isUniquelyReferencedNonObjC<T: AnyObject>(_ object: T). This replacement is > as performant as the call to isUniquelyReferenced in cases where the compiler > has static knowledge that the type of object is a native Swift class. This > change will remove surface API. > > • Swift-evolution thread: Pitch > • Swift bug: SR-1962 > • Branch with change to stdlib: remove_nonobjectivecbase > Motivation > > Today we have isUniquelyReferenced which only works on subclasses of > NonObjectiveCBase, and we have isUniquelyReferencedNonObjC which also works > on @objc classes. > > class SwiftKlazz : NonObjectiveCBase > {} > > class ObjcKlazz : > NSObject {} > > expectTrue( > isUniquelyReferenced > (SwiftKlazz())) > expectFalse( > isUniquelyReferencedNonObjC > (ObjcKlazz())) > > > // Would not compile: > > expectFalse( > isUniquelyReferenced(ObjcKlazz())) > In most cases we expect developers to be using the ManagedBufferPointer type. > In cases where they want to use a custom class they would use > isUniquelyReferenced today and can use isUniquelyReferencedNonObjC in the > future. > > class > SwiftKlazz {} > > expectTrue( > isUniquelyReferencedNonObjC(SwiftKlazz())) > Removing isUniquelyReferenced<T : NonObjectiveCBase> will allow us to remove > the NonObjectiveCBase class from the standard library thereby further > shrinking API surface. > > Proposed solution > > Remove isUniquelyReferenced<T : NonObjectiveCBase> and remove the > NonObjectiveCBase class from the standard library. Clients of the the > isUniquelyReferenced API can be migrated to use isUniquelyReferencedNonObjC. > In most cases -- where the type of the object parameter is statically known > to be a native non-@objc class -- the resulting code will have identical > performance characteristics. In cases where the type is statically not known > it will have the slight overhead of first checking that the dynamic type is > not an @objc class. > > Detailed design > > Todays APIs that can be used to check uniqueness is the family of > isUniquelyReferenced functions. > > /// Returns `true` iff `object` is a non-`@objc` class instance with > /// a single strong reference. > /// > /// * Does *not* modify `object`; the use of `inout` is an > /// implementation artifact. > /// * If `object` is an Objective-C class instance, returns `false`. > /// * Weak references do not affect the result of this function. > /// > /// Useful for implementing the copy-on-write optimization for the > /// deep storage of value types: > /// > /// mutating func modifyMe(_ arg: X) { > /// if isUniquelyReferencedNonObjC(&myStorage) { > /// myStorage.modifyInPlace(arg) > /// } > /// else { > /// myStorage = self.createModified(myStorage, arg) > /// } > /// } > public func isUniquelyReferencedNonObjC<T : AnyObject>(_ object: inout T) -> > Bool > public func isUniquelyReferencedNonObjC<T : AnyObject>(_ object: inout T?) -> > Bool > > > > /// A common base class for classes that need to be non-`@objc`, > /// recognizably in the type system. > public class NonObjectiveCBase > { > > public init > () {} > } > > > public func isUniquelyReferenced<T : NonObjectiveCBase > >( > > _ object: inout T > > ) > -> Bool > And the somewhat higher level APIs that can be used to model a storage with > several elements ManagedBufferPointer. > > /// Contains a buffer object, and provides access to an instance of > /// `Header` and contiguous storage for an arbitrary number of > /// `Element` instances stored in that buffer. > /// > /// For most purposes, the `ManagedBuffer` class works fine for this > /// purpose, and can simply be used on its own. However, in cases > /// where objects of various different classes must serve as storage, > /// `ManagedBufferPointer` is needed. > /// > /// A valid buffer class is non-`@objc`, with no declared stored > /// properties. Its `deinit` must destroy its > /// stored `Header` and any constructed `Element`s. > /// `Header` and contiguous storage for an arbitrary number of > /// `Element` instances stored in that buffer. > public struct ManagedBufferPointer<Header, Element> : Equatable > { > > /// Create with new storage containing an initial `Header` and space > > > /// for at least `minimumCapacity` `element`s. > > > /// > > > /// - parameter bufferClass: The class of the object used for storage. > > > /// - parameter minimumCapacity: The minimum number of `Element`s that > > > /// must be able to be stored in the new buffer. > > > /// - parameter initialHeader: A function that produces the initial > > > /// `Header` instance stored in the buffer, given the `buffer` > > > /// object and a function that can be called on it to get the actual > > > /// number of allocated elements. > > > /// > > > /// - Precondition: `minimumCapacity >= 0`, and the type indicated by > > > /// `bufferClass` is a non-`@objc` class with no declared stored > > > /// properties. The `deinit` of `bufferClass` must destroy its > > > /// stored `Header` and any constructed `Element`s. > > > public init > ( > bufferClass: > AnyClass > , > minimumCapacity: > Int > , > initialHeader: > @noescape (buffer: AnyObject, capacity: @noescape (AnyObject) -> Int) throws > -> > Header > ) > rethrows > > > > /// Returns `true` iff `self` holds the only strong reference to its buffer. > > > /// > > > /// See `isUniquelyReferenced` for details. > > > public mutating func holdsUniqueReference() -> Bool > > > > /// Returns `true` iff either `self` holds the only strong reference > > > /// to its buffer or the pinned has been 'pinned'. > > > /// > > > /// See `isUniquelyReferenced` for details. > > > public mutating func holdsUniqueOrPinnedReference() -> Bool > > > > internal var _nativeBuffer: Builtin. > NativeObject > } > > > /// A class whose instances contain a property of type `Header` and raw > /// storage for an array of `Element`, whose size is determined at > /// instance creation. > public class ManagedBuffer<Header, Element> > > > : ManagedProtoBuffer<Header, Element> > { > > > /// Create a new instance of the most-derived class, calling > > > /// `initialHeader` on the partially-constructed object to > > > /// generate an initial `Header`. > > > public final class func create > ( > minimumCapacity: > Int > , > initialHeader: > @noescape (ManagedProtoBuffer<Header, Element>) throws -> > Header > ) > rethrows -> ManagedBuffer<Header, Element> > { > > > let p = try ManagedBufferPointer<Header, Element> > ( > bufferClass: > self > , > minimumCapacity: minimumCapacity, > initialHeader: { buffer, _ > in > > > try > initialHeader( > > unsafeDowncast(buffer, to: ManagedProtoBuffer<Header, Element>.self > )) > }) > > > return unsafeDowncast(p.buffer, to: ManagedBuffer<Header, Element>.self > ) > } > } > > We propose to remove the NonObjectiveCBase class and isUniquelyReferenced<T: > NonObjectiveCBase>(_ object: T>. > > Impact on existing code > > Existing code that uses isUniquelyReferenced will need to remove the > NonObjectiveCBase base class and replace calls to isUniquelyReferenced by > isUniquelyReferencedNonObjC. The old API will be marked unavailable to help > migration. > > Alternatives considered > > Leave the status quo and pay for type safety with additional API surface. > Another alternative we considered -- the first version of this proposal -- > was to replace the isUniquelyReferenced API by an > isUniquelyReferencedUnsafe<T: AnyObject>(_ object: T) API that would assume > the object to be a non-@objc class and only check this precondition under > -Onone. There is however no good reason to keep this API given that the > isUniquelyReferencedNonObjC is as performant when the type is statically > known to be non-@objc class. >> On Jul 16, 2016, at 10:12 PM, Arnold via swift-evolution >> <[email protected]> wrote: >> >> Sent from my iPhone >> >> On Jul 16, 2016, at 9:41 PM, Arnold <[email protected]> wrote: >> >>> >>> >>> Sent from my iPhone >>> >>> On Jul 16, 2016, at 9:23 PM, Andrew Trick <[email protected]> wrote: >>> >>>> >>>>> On Jul 16, 2016, at 9:17 PM, Arnold <[email protected]> wrote: >>>>> >>>>> >>>>> >>>>> On Jul 16, 2016, at 8:45 PM, Andrew Trick <[email protected]> wrote: >>>>> >>>>>> >>>>>>> On Jul 16, 2016, at 8:36 PM, Arnold <[email protected]> wrote: >>>>>>> >>>>>>> Thank you for the feedback. Answers online. >>>>>>> >>>>>>> Sent from my iPhone >>>>>>> >>>>>>> On Jul 16, 2016, at 7:38 PM, Andrew Trick <[email protected]> wrote: >>>>>>> >>>>>>>> >>>>>>>>> On Jul 16, 2016, at 6:46 PM, Arnold Schwaighofer via swift-evolution >>>>>>>>> <[email protected]> wrote: >>>>>>>>> >>>>>>>>> Replace isUniquelyReferenced<T : NonObjectiveCBase> by >>>>>>>>> isUniquelyReferencedUnsafe<T: AnyObject> and remove the >>>>>>>>> NonObjectiveCBase class from the standard library. >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> So we’ll have: >>>>>>>> >>>>>>>> - isUniquelyReferencedNonObjC(object): true iff object is uniquely >>>>>>>> referenced and NonObjC >>>>>>>> >>>>>>>> - isUniquelyReferencedUnsafe(object): true iff object is uniquely >>>>>>>> reference, assert NonObjC >>>>>>>> >>>>>>>> I’m going to be picky. The “Unsafe” suffix doesn’t make sense to me. >>>>>>>> If you think this is an unsafe API then it should be: >>>>>>>> “unsafeIsUniquelyReferenced”. >>>>>>>> >>>>>>>> But I don’t really see how it is unsafe in the usual sense. If you >>>>>>>> want to convey that the programmer needs to satisfy some precondition, >>>>>>>> which is not generally associated with unsafety, then it should be: >>>>>>>> “isUniquelyReferencedAssumingNonObjC” >>>>>>>> >>>>>>> >>>>>>> Makes sense to me. I think it is unsafe in the sense if you don't >>>>>>> satisfy the precondition the resulting behavior is undefined in modes >>>>>>> other than -Onone and not checked by a precondition predicate that >>>>>>> traps. >>>>>>> >>>>>>>> unsafeIsUniquelyReferenced >>>>>>> >>>>>>> I find it kind of nice to recognize a predicate by the 'is' prefix. All >>>>>>> unsafe APIs start with the word unsafe though. I could not find an >>>>>>> unsafe freestanding predicate. >>>>>>> >>>>>>> [As the implementor of the underlying builtin you may remember that it >>>>>>> is not actually undefined and will return a implementation defined >>>>>>> value (false) for objc classes. But we might not want to guarantee this >>>>>>> going forward.] >>>>>> >>>>>> Oh yeah. I think I only kept the two versions for fear of breaking the >>>>>> API. Since you’re renaming the second one anyway, why not just delete it >>>>>> with a fixit that it's renamed to isUniquelyReferencedNonObjC? >>>>>> >>>>>> The “assuming” version of the API is extremely confusing in addition to >>>>>> being useless. >>>>> >>>>> The unsafe version would allow us to emit more efficient code (in the >>>>> future) for a static unknown object type but we know it is a native type >>>>> (but not which so we can't just cast it, this is public API so we can't >>>>> cast to Builtin.NativeObject). >>>>> >>>>> var self: AnyObject // really: AnyNativeObject >>>>> ... >>>>> if (!unsafeIsUniquelyReferenced(&self)) >>>>> self = self.copy() >>>>> } >>>>> >>>>> I admit this is somewhat contrived and am happy to just nuke the API if >>>>> we agree there is no value in the use case above. >>>> >>>> There is no sense advertising this API under some new name if it hasn’t >>>> even been implemented. The API can be added when it makes sense. >>>> >>>> +1 for eliminating it. >>> >>> Today you can implement something similar using NonObjectiveCBase as your >>> base class: >>> >>> var self: NonObjectiveCBase >>> ... >>> if (isUniquelyReferenced(&self) {...} >>> >>> And get the runtime performance of the native check. >> >> Actually, this code could just implement their own 'class NonObjCBase' base >> class and just use isUniquelyReferencedNonObj for the same performance as >> before ... >>> >>> If we implemented 'unsafeIsUniquelyReferenced' we would just unsafeBitCast >>> the 'object' argument to Builtin.NativeObject and get the same performance. >>> >>> I admit that this may be far fetched but I am trying to anticipated >>> possible use cases that work today. >>> >>> That use case will still work after nuking the API using the 'NonObjC' >>> variant albeit slightly slower. >>> >>> If we need to support it with best performance we can always bring the API >>> back as you said. >>> >>> +1 for nuking it from me >>> >>> I will change the proposal. >> _______________________________________________ >> swift-evolution mailing list >> [email protected] >> https://lists.swift.org/mailman/listinfo/swift-evolution > > _______________________________________________ > swift-evolution mailing list > [email protected] > https://lists.swift.org/mailman/listinfo/swift-evolution _______________________________________________ swift-evolution mailing list [email protected] https://lists.swift.org/mailman/listinfo/swift-evolution
