[Review] SE-0125: Remove NonObjectiveCBase and isUniquelyReferenced


(Chris Lattner) #1

Hello Swift community,

The review of "SE-0125: Remove NonObjectiveCBase and isUniquelyReferenced" begins now and runs through July 22. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0125-remove-nonobjectivecbase.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  * What is your evaluation of the proposal?
  * Is the problem being addressed significant enough to warrant a change to Swift?
  * Does this proposal fit well with the feel and direction of Swift?
  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

  https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager


(Dmitri Gribenko) #2

I like the API simplification. A few thoughts that I have after
reading the proposal that could take the simplification it even
further:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

- I find the use of different names in `isUniquelyReferenced()` and
`ManagedBufferPointer.holdsUniqueReference()` to be strange. Why not
call the latter `ManagedBufferPointer. isUniquelyReferenced()`? It is
true that we are not checking that pointer is not uniquely referenced,
if we want to emphasize that, maybe something like
`ManagedBufferPointer.isBufferUniquelyReferenced()` or
`isPointeeUniquelyReferenced()` will work?

The reason why I'm suggesting this is that `isUniquelyReferenced` and
`holdsUniqueReference` don't immediately strike me as performing the
same operation, the immediate impression I get is that these
operations are related, but subtly different, and it is not obvious
what the difference is (but after reading the docs I figure out that
there is no difference... until I need to use these APIs again).

- We have `ManagedBufferPointer.holdsUniqueOrPinnedReference()`, but
don't have an equivalent free function `isUniquelyReferencedOrPinned`
(we actually have one, but it is internal). Do we need a public one?
If we do, we could as well add it as a part of this proposal, while we
are cleaning up this library subsystem.

Dmitri

···

On Tue, Jul 19, 2016 at 10:51 PM, Chris Lattner <clattner@apple.com> wrote:

The review of "SE-0125: Remove NonObjectiveCBase and isUniquelyReferenced" begins now and runs through July 22. The proposal is available here:

        https://github.com/apple/swift-evolution/blob/master/proposals/0125-remove-nonobjectivecbase.md

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr@gmail.com>*/


(Brent Royal-Gordon) #3

I agree.

What is the reason that isUniquelyReferenced(_:slight_smile: doesn't work with Objective-C? It doesn't seem like it'd be difficult to implement—you'd either call -retainCount, or get it from the Objective-C runtime somehow, and then test it against 1—so I assume there's a reason we don't.

I ask because knowing that may help us figure out how to name it. For instance, if the issue is that we can't rely on Objective-C reference counts, we might reverse the sense of the call and name it `mayBeShared(_:)` or `mayHaveOtherReferences(_:)`.

···

On Jul 19, 2016, at 11:06 PM, Dmitri Gribenko via swift-evolution <swift-evolution@swift.org> wrote:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

--
Brent Royal-Gordon
Architechies


(Arnold Schwaighofer) #4

The review of "SE-0125: Remove NonObjectiveCBase and isUniquelyReferenced" begins now and runs through July 22. The proposal is available here:

       https://github.com/apple/swift-evolution/blob/master/proposals/0125-remove-nonobjectivecbase.md

I like the API simplification. A few thoughts that I have after
reading the proposal that could take the simplification it even
further:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

isUniquelyReferencedNonObjC checks that the object is a uniquely referenced swift only class. It works on non-@objc classes but will return false: The API will work for @objc-classes but return false.

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance.

  expectTrue(isUniquelyReferencedNonObjc(SwiftKlazz()))
  expectFalse(isUniquelyReferencedNonObjc(ObjcKlazz()))
  var y = SwiftKlazz()
  var z = [Any]
  z.append(y)
  z.append(y)
  expectFalse(isUniquelyReferencedNonObjc(y)

The simplification of this proposal just to remove the variant that had the NonObjectiveCBase prerequisite.

- I find the use of different names in `isUniquelyReferenced()` and
`ManagedBufferPointer.holdsUniqueReference()` to be strange. Why not
call the latter `ManagedBufferPointer. isUniquelyReferenced()`? It is
true that we are not checking that pointer is not uniquely referenced,
if we want to emphasize that, maybe something like
`ManagedBufferPointer.isBufferUniquelyReferenced()` or
`isPointeeUniquelyReferenced()` will work?

Okay I think isBufferUniquelyReferenced is good. I’ll add that change to the proposal.

The reason why I'm suggesting this is that `isUniquelyReferenced` and
`holdsUniqueReference` don't immediately strike me as performing the
same operation, the immediate impression I get is that these
operations are related, but subtly different, and it is not obvious
what the difference is (but after reading the docs I figure out that
there is no difference... until I need to use these APIs again).

- We have `ManagedBufferPointer.holdsUniqueOrPinnedReference()`, but
don't have an equivalent free function `isUniquelyReferencedOrPinned`
(we actually have one, but it is internal). Do we need a public one?
If we do, we could as well add it as a part of this proposal, while we
are cleaning up this library subsystem.

No we don’t one. . We have not exposed pinning outside of the standard library (because we don’t have a design for it).

I will remove it.

···

On Jul 19, 2016, at 11:06 PM, Dmitri Gribenko <gribozavr@gmail.com> wrote:
On Tue, Jul 19, 2016 at 10:51 PM, Chris Lattner <clattner@apple.com> wrote:

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr@gmail.com>*/


(Dave Abrahams) #5

The review of "SE-0125: Remove NonObjectiveCBase and
isUniquelyReferenced" begins now and runs through July 22. The
proposal is available here:

        https://github.com/apple/swift-evolution/blob/master/proposals/0125-remove-nonobjectivecbase.md

I like the API simplification. A few thoughts that I have after
reading the proposal that could take the simplification it even
further:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`?

Only if we were unable to determine that CXX imports were
uniquely-referenced.

I think that is the issue with negation. If we want to emphasize that
the API will work only with Swift types, we can do something
`isUniquelyReferencedSwiftReference`.

That's not really the point. The point is to find out whether it's
*known* to be a unique reference. False negatives are part of the
game.

       isKnownUniquelyReferenced

would work.

But I would probably suggest that we just go with just
`isUniquelyReferenced` and mention the Swift-only requirement in the
documentation.

It's not a requirement, as in “precondition.” The point of having
`NonObjC` in this API is actually that it *does* work on ObjC
references, by returning false.

- I find the use of different names in `isUniquelyReferenced()` and
`ManagedBufferPointer.holdsUniqueReference()` to be strange. Why not
call the latter `ManagedBufferPointer. isUniquelyReferenced()`? It is
true that we are not checking that pointer is not uniquely referenced,
if we want to emphasize that, maybe something like
`ManagedBufferPointer.isBufferUniquelyReferenced()` or
`isPointeeUniquelyReferenced()` will work?

       isUniqueReference

would work for ManagedBufferPointer.

The reason why I'm suggesting this is that `isUniquelyReferenced` and
`holdsUniqueReference` don't immediately strike me as performing the
same operation, the immediate impression I get is that these
operations are related, but subtly different, and it is not obvious
what the difference is (but after reading the docs I figure out that
there is no difference... until I need to use these APIs again).

- We have `ManagedBufferPointer.holdsUniqueOrPinnedReference()`, but
don't have an equivalent free function `isUniquelyReferencedOrPinned`
(we actually have one, but it is internal). Do we need a public one?
If we do, we could as well add it as a part of this proposal, while we
are cleaning up this library subsystem.

Maybe, but it's not crucial.

···

on Tue Jul 19 2016, Dmitri Gribenko <gribozavr-AT-gmail.com> wrote:

On Tue, Jul 19, 2016 at 10:51 PM, Chris Lattner <clattner@apple.com> wrote:

--
Dave


(Arnold Schwaighofer) #6

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

I agree.

What is the reason that isUniquelyReferenced(_:slight_smile: doesn't work with Objective-C? It doesn't seem like it'd be difficult to implement—you'd either call -retainCount, or get it from the Objective-C runtime somehow, and then test it against 1—so I assume there's a reason we don’t

Also, see my answer to Dmitri.

isUniquelyReferencedNonObjC checks that the object is a uniquely referenced swift only class. It works on non-@objc classes but will return false: The API will work for @objc-classes but return false.

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance. (You can’t store to an NSArray for example, you would have to check it was a NSMutableArray, etc …)

···

On Jul 20, 2016, at 3:13 AM, Brent Royal-Gordon <brent@architechies.com> wrote:

On Jul 19, 2016, at 11:06 PM, Dmitri Gribenko via swift-evolution <swift-evolution@swift.org> wrote:

I ask because knowing that may help us figure out how to name it. For instance, if the issue is that we can't rely on Objective-C reference counts, we might reverse the sense of the call and name it `mayBeShared(_:)` or `mayHaveOtherReferences(_:)`.

--
Brent Royal-Gordon
Architechies


(Arnold Schwaighofer) #7

The review of "SE-0125: Remove NonObjectiveCBase and isUniquelyReferenced" begins now and runs through July 22. The proposal is available here:

      https://github.com/apple/swift-evolution/blob/master/proposals/0125-remove-nonobjectivecbase.md

I like the API simplification. A few thoughts that I have after
reading the proposal that could take the simplification it even
further:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

isUniquelyReferencedNonObjC checks that the object is a uniquely referenced swift only class. It works on non-@objc classes but will return false: The API will work for @objc-classes but return false.

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance.

expectTrue(isUniquelyReferencedNonObjc(SwiftKlazz()))
expectFalse(isUniquelyReferencedNonObjc(ObjcKlazz()))
var y = SwiftKlazz()
var z = [Any]
z.append(y)
z.append(y)
expectFalse(isUniquelyReferencedNonObjc(y)

The simplification of this proposal just to remove the variant that had the NonObjectiveCBase prerequisite.

Your critique of the negation still holds though.

So maybe we still rename it from isUniquelyReferencedNonObjC to isUniquelyReferencedSwiftReference?

···

On Jul 20, 2016, at 5:55 AM, Arnold Schwaighofer via swift-evolution <swift-evolution@swift.org> wrote:

On Jul 19, 2016, at 11:06 PM, Dmitri Gribenko <gribozavr@gmail.com> wrote:
On Tue, Jul 19, 2016 at 10:51 PM, Chris Lattner <clattner@apple.com> wrote:

- I find the use of different names in `isUniquelyReferenced()` and
`ManagedBufferPointer.holdsUniqueReference()` to be strange. Why not
call the latter `ManagedBufferPointer. isUniquelyReferenced()`? It is
true that we are not checking that pointer is not uniquely referenced,
if we want to emphasize that, maybe something like
`ManagedBufferPointer.isBufferUniquelyReferenced()` or
`isPointeeUniquelyReferenced()` will work?

Okay I think isBufferUniquelyReferenced is good. I’ll add that change to the proposal.

The reason why I'm suggesting this is that `isUniquelyReferenced` and
`holdsUniqueReference` don't immediately strike me as performing the
same operation, the immediate impression I get is that these
operations are related, but subtly different, and it is not obvious
what the difference is (but after reading the docs I figure out that
there is no difference... until I need to use these APIs again).

- We have `ManagedBufferPointer.holdsUniqueOrPinnedReference()`, but
don't have an equivalent free function `isUniquelyReferencedOrPinned`
(we actually have one, but it is internal). Do we need a public one?
If we do, we could as well add it as a part of this proposal, while we
are cleaning up this library subsystem.

No we don’t one. . We have not exposed pinning outside of the standard library (because we don’t have a design for it).

I will remove it.

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr@gmail.com>*/

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Dave Abrahams) #8

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

I agree.

What is the reason that isUniquelyReferenced(_:slight_smile: doesn't work with
Objective-C? It doesn't seem like it'd be difficult to implement—you'd
either call -retainCount, or get it from the Objective-C runtime
somehow, and then test it against 1—so I assume there's a reason we
don't.

There is; it's basically impossible to implement reliably given the
state of Objective-C reference-counting. I'll let Greg Parker explain
the details, as he has many times to me (but I don't retain them as well
as he does).

···

on Wed Jul 20 2016, Brent Royal-Gordon <brent-AT-architechies.com> wrote:

On Jul 19, 2016, at 11:06 PM, Dmitri Gribenko via swift-evolution <swift-evolution@swift.org> wrote:

I ask because knowing that may help us figure out how to name it. For
instance, if the issue is that we can't rely on Objective-C reference
counts, we might reverse the sense of the call and name it
`mayBeShared(_:)` or `mayHaveOtherReferences(_:)`.

--
Dave


(Arnold Schwaighofer) #9

The review of "SE-0125: Remove NonObjectiveCBase and
isUniquelyReferenced" begins now and runs through July 22. The
proposal is available here:

       https://github.com/apple/swift-evolution/blob/master/proposals/0125-remove-nonobjectivecbase.md

I like the API simplification. A few thoughts that I have after
reading the proposal that could take the simplification it even
further:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`?

Only if we were unable to determine that CXX imports were
uniquely-referenced.

I think that is the issue with negation. If we want to emphasize that
the API will work only with Swift types, we can do something
`isUniquelyReferencedSwiftReference`.

That's not really the point. The point is to find out whether it's
*known* to be a unique reference. False negatives are part of the
game.

      isKnownUniquelyReferenced

would work.

This would be a change to the semantics of the API. Currently, you can rely on -- and is documented as -- that if it returns true you can assume the reference points to a non-@objc instance.

Users may be relying on this behavior.

Do we want to add an isNativeSwiftReference API?

···

Sent from my iPhone

On Jul 20, 2016, at 6:50 AM, Dave Abrahams <dabrahams@apple.com> wrote:

on Tue Jul 19 2016, Dmitri Gribenko <gribozavr-AT-gmail.com> wrote:

On Tue, Jul 19, 2016 at 10:51 PM, Chris Lattner <clattner@apple.com> wrote:

But I would probably suggest that we just go with just
`isUniquelyReferenced` and mention the Swift-only requirement in the
documentation.

It's not a requirement, as in “precondition.” The point of having
`NonObjC` in this API is actually that it *does* work on ObjC
references, by returning false.

- I find the use of different names in `isUniquelyReferenced()` and
`ManagedBufferPointer.holdsUniqueReference()` to be strange. Why not
call the latter `ManagedBufferPointer. isUniquelyReferenced()`? It is
true that we are not checking that pointer is not uniquely referenced,
if we want to emphasize that, maybe something like
`ManagedBufferPointer.isBufferUniquelyReferenced()` or
`isPointeeUniquelyReferenced()` will work?

      isUniqueReference

would work for ManagedBufferPointer.

The reason why I'm suggesting this is that `isUniquelyReferenced` and
`holdsUniqueReference` don't immediately strike me as performing the
same operation, the immediate impression I get is that these
operations are related, but subtly different, and it is not obvious
what the difference is (but after reading the docs I figure out that
there is no difference... until I need to use these APIs again).

- We have `ManagedBufferPointer.holdsUniqueOrPinnedReference()`, but
don't have an equivalent free function `isUniquelyReferencedOrPinned`
(we actually have one, but it is internal). Do we need a public one?
If we do, we could as well add it as a part of this proposal, while we
are cleaning up this library subsystem.

Maybe, but it's not crucial.

--
Dave


(Haravikk) #10

I'm kind of undecided too; in fact it's never been clear to me why this is even a global function at all. In my mind the best solution would be some kind of ReferenceCounted class protocol with a .isUniquelyReferenced computed property, thus eliminating the global function entirely. The pure-Swift base type would conform to this protocol and provide the ability to test its uniqueness directly, though a static method could also be provided for testing unknown types as well (but I feel this is a relatively uncommon case), and the protocol conformance could be added to an Objective-C, C++ and so-on base-types whenever these are possible.

It also strikes me as odd that we have to specify a NonObjectiveCBase in any form (extension or function, doesn't really matter); I would think that a pure Swift base should be the default in most cases, with a distinction having to be made only when declaring public classes in a module with Objective-C bridging headers. In other words it feels strange that non-Objective-C is the exception, when the goal should be trying to write pure Swift code wherever possible.

I don't know about anyone else, but I mostly only use NonObjectiveCBase on private types implementing copy-on-write behaviour, so my only use-case for isUniquelyReferenced is private to the module anyway, so shouldn't really raise an issue of whether it's Objective-C compatible.

···

On 20 Jul 2016, at 11:13, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

On Jul 19, 2016, at 11:06 PM, Dmitri Gribenko via swift-evolution <swift-evolution@swift.org> wrote:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

I agree.

What is the reason that isUniquelyReferenced(_:slight_smile: doesn't work with Objective-C? It doesn't seem like it'd be difficult to implement—you'd either call -retainCount, or get it from the Objective-C runtime somehow, and then test it against 1—so I assume there's a reason we don't.

I ask because knowing that may help us figure out how to name it. For instance, if the issue is that we can't rely on Objective-C reference counts, we might reverse the sense of the call and name it `mayBeShared(_:)` or `mayHaveOtherReferences(_:)`.


(Arnold Schwaighofer) #11

As mentioned in the other emails isUniquelyReferencedNonObjC works on @objc classes.

What has been misunderstood is the answer it returns. It returns false for @objc classes. Also see my other answers.

expectTrue(isUniquelyReferencedNonObjc(SwiftKlazz()))
expectFalse(isUniquelyReferencedNonObjc(ObjcKlazz()))
var y = SwiftKlazz()
var z = [Any]
z.append(y)
z.append(y)
expectFalse(isUniquelyReferencedNonObjc(y)

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance.

isUniquelyReferencedNonObjC <T: NonObjectiveBase> was there as a separate function historically. Because it was faster. This has changed and there is no need to have both anymore.

As mentioned in the proposal you can use isUniquelyReferencedNonObjC where you are using isUniquelyReferencedNonObjC today. And it is semantically equivalent and has the same performance.

···

On Jul 20, 2016, at 5:03 AM, Haravikk via swift-evolution <swift-evolution@swift.org> wrote:

On 20 Jul 2016, at 11:13, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

On Jul 19, 2016, at 11:06 PM, Dmitri Gribenko via swift-evolution <swift-evolution@swift.org> wrote:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`? I think that is the issue
with negation. If we want to emphasize that the API will work only
with Swift types, we can do something
`isUniquelyReferencedSwiftReference`. But I would probably suggest
that we just go with just `isUniquelyReferenced` and mention the
Swift-only requirement in the documentation.

I agree.

What is the reason that isUniquelyReferenced(_:slight_smile: doesn't work with Objective-C? It doesn't seem like it'd be difficult to implement—you'd either call -retainCount, or get it from the Objective-C runtime somehow, and then test it against 1—so I assume there's a reason we don't.

I ask because knowing that may help us figure out how to name it. For instance, if the issue is that we can't rely on Objective-C reference counts, we might reverse the sense of the call and name it `mayBeShared(_:)` or `mayHaveOtherReferences(_:)`.

I'm kind of undecided too; in fact it's never been clear to me why this is even a global function at all. In my mind the best solution would be some kind of ReferenceCounted class protocol with a .isUniquelyReferenced computed property, thus eliminating the global function entirely. The pure-Swift base type would conform to this protocol and provide the ability to test its uniqueness directly, though a static method could also be provided for testing unknown types as well (but I feel this is a relatively uncommon case), and the protocol conformance could be added to an Objective-C, C++ and so-on base-types whenever these are possible.

It also strikes me as odd that we have to specify a NonObjectiveCBase in any form (extension or function, doesn't really matter); I would think that a pure Swift base should be the default in most cases, with a distinction having to be made only when declaring public classes in a module with Objective-C bridging headers. In other words it feels strange that non-Objective-C is the exception, when the goal should be trying to write pure Swift code wherever possible.

I don't know about anyone else, but I mostly only use NonObjectiveCBase on private types implementing copy-on-write behaviour, so my only use-case for isUniquelyReferenced is private to the module anyway, so shouldn't really raise an issue of whether it's Objective-C compatible.


(Brent Royal-Gordon) #12

It sounds to me like this is specifically tuned to the bridging the standard library happens to do, but you could very well want the opposite behavior in other cases. (For instance, `NSCountedSet` and `NSURLComponents` are both always-mutable; COW structs built around them would want to examine their refcounts.)

Is this the design we want to publish to users of the standard library? Or would it be better to have an `isNonObjectiveC(_:)` call and an `isUniquelyReferenced(_:)` call, and perhaps wrap them into one call for the standard library's internal use?

···

On Jul 20, 2016, at 5:59 AM, Arnold Schwaighofer <aschwaighofer@apple.com> wrote:

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance. (You can’t store to an NSArray for example, you would have to check it was a NSMutableArray, etc …)

--
Brent Royal-Gordon
Architechies


(Brent Royal-Gordon) #13

I don't necessarily need to know them (although I'd be curious to hear).

I think something that implies the possibility of false negatives, like `isKnownUniquelyReferenced(_:)` or (flipping the sense of the return value) `mayBeShared(_:)`, is probably the best strategy. Or, if COW is the only use case we want to support, we could even call it something along the lines of `needsCopyBeforeMutating(_:)`, which would strongly imply it was unsuitable for any other purpose.

···

On Jul 20, 2016, at 6:53 AM, Dave Abrahams <dave@boostpro.com> wrote:

There is; it's basically impossible to implement reliably given the
state of Objective-C reference-counting. I'll let Greg Parker explain
the details, as he has many times to me (but I don't retain them as well
as he does).

--
Brent Royal-Gordon
Architechies


(Tony Parker) #14

For what it’s worth, I agree with Dmitri on the naming of this function. I think we should find a way to give it a name without “NonObjC” in it.

- Tony

···

On Jul 20, 2016, at 8:15 AM, Arnold via swift-evolution <swift-evolution@swift.org> wrote:

Sent from my iPhone

On Jul 20, 2016, at 6:50 AM, Dave Abrahams <dabrahams@apple.com> wrote:

on Tue Jul 19 2016, Dmitri Gribenko <gribozavr-AT-gmail.com> wrote:

On Tue, Jul 19, 2016 at 10:51 PM, Chris Lattner <clattner@apple.com> wrote:
The review of "SE-0125: Remove NonObjectiveCBase and
isUniquelyReferenced" begins now and runs through July 22. The
proposal is available here:

      https://github.com/apple/swift-evolution/blob/master/proposals/0125-remove-nonobjectivecbase.md

I like the API simplification. A few thoughts that I have after
reading the proposal that could take the simplification it even
further:

- I find it a little strange to see a mention of Objective-C, and a
negation in `isUniquelyReferencedNonObjC`. For example, if we get C++
import, would we need to rename it to
`isUniquelyReferencedNonObjCAndNonCXX`?

Only if we were unable to determine that CXX imports were
uniquely-referenced.

I think that is the issue with negation. If we want to emphasize that
the API will work only with Swift types, we can do something
`isUniquelyReferencedSwiftReference`.

That's not really the point. The point is to find out whether it's
*known* to be a unique reference. False negatives are part of the
game.

     isKnownUniquelyReferenced

would work.

This would be a change to the semantics of the API. Currently, you can rely on -- and is documented as -- that if it returns true you can assume the reference points to a non-@objc instance.

Users may be relying on this behavior.

Do we want to add an isNativeSwiftReference API?

But I would probably suggest that we just go with just
`isUniquelyReferenced` and mention the Swift-only requirement in the
documentation.

It's not a requirement, as in “precondition.” The point of having
`NonObjC` in this API is actually that it *does* work on ObjC
references, by returning false.

- I find the use of different names in `isUniquelyReferenced()` and
`ManagedBufferPointer.holdsUniqueReference()` to be strange. Why not
call the latter `ManagedBufferPointer. isUniquelyReferenced()`? It is
true that we are not checking that pointer is not uniquely referenced,
if we want to emphasize that, maybe something like
`ManagedBufferPointer.isBufferUniquelyReferenced()` or
`isPointeeUniquelyReferenced()` will work?

     isUniqueReference

would work for ManagedBufferPointer.

The reason why I'm suggesting this is that `isUniquelyReferenced` and
`holdsUniqueReference` don't immediately strike me as performing the
same operation, the immediate impression I get is that these
operations are related, but subtly different, and it is not obvious
what the difference is (but after reading the docs I figure out that
there is no difference... until I need to use these APIs again).

- We have `ManagedBufferPointer.holdsUniqueOrPinnedReference()`, but
don't have an equivalent free function `isUniquelyReferencedOrPinned`
(we actually have one, but it is internal). Do we need a public one?
If we do, we could as well add it as a part of this proposal, while we
are cleaning up this library subsystem.

Maybe, but it's not crucial.

--
Dave

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Andrew Trick) #15

What’s the difference between a “SwiftReference” and AnyObject?

The current name is the literal meaning. The intent of being literal and awkward is to encourage users to read the comments. This isn’t something people should be embedding in their program logic anyway. It’s very special purpose.

That said, something like “isUniquelyReferencedNativeSwift” would work assuming that’s semantically correct (“native" Swfit objects do not inherit from NSObject). “isKnownUniquelyReferenced” would be fine with a warning in the doc comment that it may always return false for objc objects.

Andy

···

On Jul 20, 2016, at 6:12 AM, Arnold Schwaighofer via swift-evolution <swift-evolution@swift.org> wrote:

isUniquelyReferencedNonObjC checks that the object is a uniquely referenced swift only class. It works on non-@objc classes but will return false: The API will work for @objc-classes but return false.

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance.

expectTrue(isUniquelyReferencedNonObjc(SwiftKlazz()))
expectFalse(isUniquelyReferencedNonObjc(ObjcKlazz()))
var y = SwiftKlazz()
var z = [Any]
z.append(y)
z.append(y)
expectFalse(isUniquelyReferencedNonObjc(y)

The simplification of this proposal just to remove the variant that had the NonObjectiveCBase prerequisite.

Your critique of the negation still holds though.

So maybe we still rename it from isUniquelyReferencedNonObjC to isUniquelyReferencedSwiftReference?


(Arnold Schwaighofer) #16

On Darwin and iOS systems it would be hard to implement a uniques check efficiently for objective c objects.

You can't rely on retainCount is what I remember. NonPointer ISA, slide tables that contain weak and actual reference counts would make it hard to make such a check efficient.

This would certainly be out of scope for swift 3

We can add the API you propose later.

It has different semantics to the existing API.

···

Sent from my iPhone

On Jul 20, 2016, at 6:22 AM, Brent Royal-Gordon <brent@architechies.com> wrote:

On Jul 20, 2016, at 5:59 AM, Arnold Schwaighofer <aschwaighofer@apple.com> wrote:

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance. (You can’t store to an NSArray for example, you would have to check it was a NSMutableArray, etc …)

It sounds to me like this is specifically tuned to the bridging the standard library happens to do, but you could very well want the opposite behavior in other cases. (For instance, `NSCountedSet` and `NSURLComponents` are both always-mutable; COW structs built around them would want to examine their refcounts.)

Is this the design we want to publish to users of the standard library? Or would it be better to have an `isNonObjectiveC(_:)` call and an `isUniquelyReferenced(_:)` call, and perhaps wrap them into one call for the standard library's internal use?

--
Brent Royal-Gordon
Architechies


(Dave Abrahams) #17

There is; it's basically impossible to implement reliably given the
state of Objective-C reference-counting. I'll let Greg Parker explain
the details, as he has many times to me (but I don't retain them as well
as he does).

I don't necessarily need to know them (although I'd be curious to hear).

I think something that implies the possibility of false negatives,
like `isKnownUniquelyReferenced(_:)` or (flipping the sense of the
return value) `mayBeShared(_:)`, is probably the best strategy. Or, if
COW is the only use case we want to support,

It's not the only use case for ManagedBuffer[Pointer], if that's what
you mean.

···

on Wed Jul 20 2016, Brent Royal-Gordon <swift-evolution@swift.org> wrote:

On Jul 20, 2016, at 6:53 AM, Dave Abrahams <dave@boostpro.com> wrote:

we could even call it something along the lines of
`needsCopyBeforeMutating(_:)`, which would strongly imply it was
unsuitable for any other purpose.

--
Dave


(Arnold Schwaighofer) #18

isUniquelyReferencedNonObjC checks that the object is a uniquely referenced swift only class. It works on non-@objc classes but will return false: The API will work for @objc-classes but return false.

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance.

expectTrue(isUniquelyReferencedNonObjc(SwiftKlazz()))
expectFalse(isUniquelyReferencedNonObjc(ObjcKlazz()))
var y = SwiftKlazz()
var z = [Any]
z.append(y)
z.append(y)
expectFalse(isUniquelyReferencedNonObjc(y)

The simplification of this proposal just to remove the variant that had the NonObjectiveCBase prerequisite.

Your critique of the negation still holds though.

So maybe we still rename it from isUniquelyReferencedNonObjC to isUniquelyReferencedSwiftReference?

What’s the difference between a “SwiftReference” and AnyObject?

The current name is the literal meaning. The intent of being literal and awkward is to encourage users to read the comments. This isn’t something people should be embedding in their program logic anyway. It’s very special purpose.

The question is are they relying on the non-@objc post-condition when the API returns true? If they were to implement something like array they might.

That said, something like “isUniquelyReferencedNativeSwift” would work assuming that’s semantically correct (“native" Swfit objects do not inherit from NSObject). “isKnownUniquelyReferenced” would be fine with a warning in the doc comment that it may always return false for objc objects.

Native swift objects are the ones that use native swift reference counting and don’t alias Objc class instances. That is at least how we have defined it at the SIL (Builtin.NativeObject vs Builtin.UnknownObject) level:

* A ``Builtin.NativeObject`` may alias any native Swift heap object,
  including a Swift class instance, a box allocated by ``alloc_box``,
  or a thick function's closure context.
  It may not alias natively Objective-C class instances.

I think at the language/stdlib level the “native” concept is implementation detail that is not witnessed other than with the non-@objc requirement of ManageBufferPointer and isUniquelyReferencedNonObjC, i.e at the language/stdlib level we call “native” "non-@objc”. Which IMO is more descriptive.

I understand the desire to remove Objc’ness from API names that can be used on platforms without ObjC.

···

On Jul 20, 2016, at 9:09 AM, Andrew Trick <atrick@apple.com> wrote:

On Jul 20, 2016, at 6:12 AM, Arnold Schwaighofer via swift-evolution <swift-evolution@swift.org> wrote:


(Jaden Geller) #19

I agree that `isKnownUniquelyReferenced` (with a doc comment about Objective-C) seems like a reasonable solution. It feels bizarre to name Objective-C in a Swift standard library function name. If I recall correctly, Swift on Linux doesn’t interop with Objective-C, so this name feels very out of place there. I’m definitely +1 for any solution that drops this negation from the function name and moves it to the doc comment.

···

On Jul 20, 2016, at 9:09 AM, Andrew Trick via swift-evolution <swift-evolution@swift.org> wrote:

On Jul 20, 2016, at 6:12 AM, Arnold Schwaighofer via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

isUniquelyReferencedNonObjC checks that the object is a uniquely referenced swift only class. It works on non-@objc classes but will return false: The API will work for @objc-classes but return false.

The reason for this combination is data structures like Array which might hold an objc class which is not mutable. On a mutating operation you check is it uniquely reference and a swift class - if the answer is yes and you have enough storage you store to the current instance.

expectTrue(isUniquelyReferencedNonObjc(SwiftKlazz()))
expectFalse(isUniquelyReferencedNonObjc(ObjcKlazz()))
var y = SwiftKlazz()
var z = [Any]
z.append(y)
z.append(y)
expectFalse(isUniquelyReferencedNonObjc(y)

The simplification of this proposal just to remove the variant that had the NonObjectiveCBase prerequisite.

Your critique of the negation still holds though.

So maybe we still rename it from isUniquelyReferencedNonObjC to isUniquelyReferencedSwiftReference?

What’s the difference between a “SwiftReference” and AnyObject?

The current name is the literal meaning. The intent of being literal and awkward is to encourage users to read the comments. This isn’t something people should be embedding in their program logic anyway. It’s very special purpose.

That said, something like “isUniquelyReferencedNativeSwift” would work assuming that’s semantically correct (“native" Swfit objects do not inherit from NSObject). “isKnownUniquelyReferenced” would be fine with a warning in the doc comment that it may always return false for objc objects.

Andy
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Dave Abrahams) #20

isUniquelyReferencedNonObjC checks that the object is a uniquely
referenced swift only class. It works on non-@objc classes but will
return false: The API will work for @objc-classes but return false.

The reason for this combination is data structures like Array which
might hold an objc class which is not mutable. On a mutating
operation you check is it uniquely reference and a swift class - if
the answer is yes and you have enough storage you store to the
current instance.

expectTrue(isUniquelyReferencedNonObjc(SwiftKlazz()))
expectFalse(isUniquelyReferencedNonObjc(ObjcKlazz()))
var y = SwiftKlazz()
var z = [Any]
z.append(y)
z.append(y)
expectFalse(isUniquelyReferencedNonObjc(y)

The simplification of this proposal just to remove the variant that had the NonObjectiveCBase prerequisite.

Your critique of the negation still holds though.

So maybe we still rename it from isUniquelyReferencedNonObjC to isUniquelyReferencedSwiftReference?

What’s the difference between a “SwiftReference” and AnyObject?

The current name is the literal meaning. The intent of being literal
and awkward is to encourage users to read the comments. This isn’t
something people should be embedding in their program logic
anyway. It’s very special purpose.

That said, something like “isUniquelyReferencedNativeSwift” would work
assuming that’s semantically correct (“native" Swfit objects do not
inherit from NSObject).

It's semantically correct today but I'm not sure it always be. If we
decided interop with Python, for example, we'd be able to detect
uniqueness of Python objects and this method should do so.

“isKnownUniquelyReferenced” would be fine with a warning in the doc
comment that it may always return false for objc objects.

That's a better name.

P.S. It's conceivable that someday we could correctly detect uniqueness
in (a subset of) ObjC types.

···

on Wed Jul 20 2016, Andrew Trick <atrick-AT-apple.com> wrote:

On Jul 20, 2016, at 6:12 AM, Arnold Schwaighofer via swift-evolution <swift-evolution@swift.org> wrote:

--
Dave