[Review] SE-0017: Change Unmanaged to use UnsafePointer


(Chris Lattner) #1

Hello Swift community,

The review of "SE-0017: Change Unmanaged to use UnsafePointer" begins now and runs through May 3. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.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, eventually, determine 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 you 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


(John McCall) #2

Hello Swift community,

The review of "SE-0017: Change Unmanaged to use UnsafePointer" begins now and runs through May 3. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.md

This is a really unfortunate proposal title. I would suggest "Add conversions directly between Unmanaged and UnsafePointer<Void>".

John.

···

On Apr 28, 2016, at 11:10 AM, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

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, eventually, determine 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 you 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

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


(Andrew Trick) #3

I have some concerns, but let me just suggest a simple alternative and see what people think...

- Leave the existing from/toOpaque API until we can come up with a better plan for moving away from OpaquePointer.

- Add initializers to avoid boilerplate, but only for "safe" variants of the cast:

extension Unmanaged {
  @_transparent
  public init(_ from : UnsafePointer<Instance>)

  @_transparent
  public init?(_ from : UnsafePointer<Instance>?)
}

extension UnsafeMutablePointer where Pointee : AnyObject {
  @_transparent
  public init(_ from : Unmanaged<Pointee>)

  @_transparent
  public init?(_ from : Unmanaged<Pointee>?)
}

- This doesn't solve the stated problem of passing unmanaged pointers to 'void*' imports. Is that really an issue? I believe the correct fix is to stop importing 'void*' as UnsafePointer<Void>. We should have a nominally distinct "opaque" pointer type, 'void*' should be imported as that type, and casting from any UnsafePointer to the opaque pointer type should be inferred and implicit for function arguments. I can send a proposal for eliminating UnsafePointer<Void> next week, but the scope of that proposal will be much broader.

-Andy

···

On Apr 28, 2016, at 11:10 AM, Chris Lattner <clattner@apple.com> wrote:

Hello Swift community,

The review of "SE-0017: Change Unmanaged to use UnsafePointer" begins now and runs through May 3. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.md


(Patrick Smith) #4

* What is your evaluation of the proposal?This sounds like a great move. I recently used the CFNotificationCenter APIs, where I needed an UnsafePointer from an Unmanaged and the process was a bit surprising. Good to know first off I was even doing it the correct way, and I think this change substantially improves it.
  * Is the problem being addressed significant enough to warrant a change to Swift?Yes, I imagine the use case of a pointer is the most wanted.
  * Does this proposal fit well with the feel and direction of Swift?Each part with Unmanaged and UnsafePointer seem well thought out and logical, and this change fits in well.
  * If you have you used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?The only language I can think of is Objective-C, and I think this matches the flow of __bridge and const void * well.
Patrick


(Haravikk) #5

  * What is your evaluation of the proposal?

Positive, while working with pointers should be avoided, when you have no choice it’s better for them to be as easy as possible to minimise errors, even if we can’t eliminate them completely (since that’s the risk with pointers).

  * Is the problem being addressed significant enough to warrant a change to Swift?

Yes. While the conversion is trivial, it’s annoying, and essentially involves redundant types, so it’s better to eliminate them entirely.

  * Does this proposal fit well with the feel and direction of Swift?

Yes. While Swift tries to avoid pointers, when they are needed they should be as simple and uncluttered as possible.

  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick skim, but it’s pretty self-explanatory.

···

On 28 Apr 2016, at 19:10, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:


(Lily Ballard) #6

Hello Swift community,

The review of "SE-0017: Change Unmanaged to use UnsafePointer" begins now and runs through May 3. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.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, eventually, determine 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?

+1

  * Is the problem being addressed significant enough to warrant a change to Swift?

Yes. Going through COpaquePointer is quite annoying when dealing with Unmanaged and C context pointers.

  * Does this proposal fit well with the feel and direction of Swift?

Yes.

  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

A quick reading, and I've hit this pain point in my own code before.

-Kevin Ballard

···

On Thu, Apr 28, 2016, at 11:10 AM, Chris Lattner wrote:


(Jordan Rose) #7

Hello Swift community,

The review of "SE-0017: Change Unmanaged to use UnsafePointer" begins now and runs through May 3. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.md

I have some concerns, but let me just suggest a simple alternative and see what people think...

- Leave the existing from/toOpaque API until we can come up with a better plan for moving away from OpaquePointer.

- Add initializers to avoid boilerplate, but only for "safe" variants of the cast:

extension Unmanaged {
@_transparent
public init(_ from : UnsafePointer<Instance>)

@_transparent
public init?(_ from : UnsafePointer<Instance>?)
}

extension UnsafeMutablePointer where Pointee : AnyObject {
@_transparent
public init(_ from : Unmanaged<Pointee>)

@_transparent
public init?(_ from : Unmanaged<Pointee>?)
}

This isn’t correct; an UnsafeMutablePointer<Foo> is a pointer to a reference to Foo. Unmanaged<Foo> is a wrapper around ‘unowned Foo’, i.e. it’s just the reference.

- This doesn't solve the stated problem of passing unmanaged pointers to 'void*' imports. Is that really an issue? I believe the correct fix is to stop importing 'void*' as UnsafePointer<Void>. We should have a nominally distinct "opaque" pointer type, 'void*' should be imported as that type, and casting from any UnsafePointer to the opaque pointer type should be inferred and implicit for function arguments. I can send a proposal for eliminating UnsafePointer<Void> next week, but the scope of that proposal will be much broader.

This is one of the few major use cases for Unmanaged: passing objects through C context pointers. If the type of a ‘void *’ pointer changes, then this proposal should use that type.

(The other supported uses of Unmanaged are interacting with existing CF APIs that haven’t been audited, and dealing with fields of structs with class type, neither of which use fromOpaque/toOpaque. That last actually isn’t implemented correctly at the moment; we’re assuming those are all strong references, which they aren’t.)

Jordan

···

On Apr 28, 2016, at 17:22, Andrew Trick via swift-evolution <swift-evolution@swift.org> wrote:

On Apr 28, 2016, at 11:10 AM, Chris Lattner <clattner@apple.com> wrote:


(Andrew Trick) #8

Thanks. Now I understand the purpose of this proposal. My concern was that UnsafePointer<Void> is probably not the right type for opaque pointers. I didn’t want users to begin rewriting COpaquePointer APIs with UnsafePointer<Void>, then force them to rewrite the same calls again back to some other opaque pointer type once we decide what that should be. I was hoping to sidestep my concerns and meet the goal of reduced syntax, but my suggestion was nonsense.

I can start a separate thread next week on replacing UnsafePointer<Void> and see where that goes. This proposal is probably fine as-is, but there might be less impact for users if we change the imported void* type first.

-Andy

···

On Apr 29, 2016, at 5:10 PM, Jordan Rose <jordan_rose@apple.com> wrote:

On Apr 28, 2016, at 17:22, Andrew Trick via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

On Apr 28, 2016, at 11:10 AM, Chris Lattner <clattner@apple.com <mailto:clattner@apple.com>> wrote:

Hello Swift community,

The review of "SE-0017: Change Unmanaged to use UnsafePointer" begins now and runs through May 3. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.md

I have some concerns, but let me just suggest a simple alternative and see what people think...

- Leave the existing from/toOpaque API until we can come up with a better plan for moving away from OpaquePointer.

- Add initializers to avoid boilerplate, but only for "safe" variants of the cast:

extension Unmanaged {
@_transparent
public init(_ from : UnsafePointer<Instance>)

@_transparent
public init?(_ from : UnsafePointer<Instance>?)
}

extension UnsafeMutablePointer where Pointee : AnyObject {
@_transparent
public init(_ from : Unmanaged<Pointee>)

@_transparent
public init?(_ from : Unmanaged<Pointee>?)
}

This isn’t correct; an UnsafeMutablePointer<Foo> is a pointer to a reference to Foo. Unmanaged<Foo> is a wrapper around ‘unowned Foo’, i.e. it’s just the reference.

- This doesn't solve the stated problem of passing unmanaged pointers to 'void*' imports. Is that really an issue? I believe the correct fix is to stop importing 'void*' as UnsafePointer<Void>. We should have a nominally distinct "opaque" pointer type, 'void*' should be imported as that type, and casting from any UnsafePointer to the opaque pointer type should be inferred and implicit for function arguments. I can send a proposal for eliminating UnsafePointer<Void> next week, but the scope of that proposal will be much broader.

This is one of the few major use cases for Unmanaged: passing objects through C context pointers. If the type of a ‘void *’ pointer changes, then this proposal should use that type.

(The other supported uses of Unmanaged are interacting with existing CF APIs that haven’t been audited, and dealing with fields of structs with class type, neither of which use fromOpaque/toOpaque. That last actually isn’t implemented correctly at the moment; we’re assuming those are all strong references, which they aren’t.)