[Review] SE-0147: Move UnsafeMutablePointer.initialize(from:) to UnsafeMutableBufferPointer


(Douglas Gregor) #1

Hello Swift community,

The review of SE-0147 "Move UnsafeMutablePointer.initialize(from:) to UnsafeMutableBufferPointer" begins now and runs through December 12, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.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. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.md
Reply text

Other replies
<https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.md#what-goes-into-a-review-1>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 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,

-Doug

Review Manager


(Alex Martini) #2

extension UnsafeMutableBufferPointer {
  /// Initializes memory in the buffer with the elements of `source`.
  /// Returns an iterator to any elements of `source` that didn't fit in the
  /// buffer, and an index to the point in the buffer one past the last element
  /// written (so `startIndex` if no elements written, `endIndex` if the buffer
  /// was completely filled).
  ///
  /// - Precondition: The memory in `self` is uninitialized. The buffer must contain
  /// sufficient uninitialized memory to accommodate `source.underestimatedCount`.
  ///
  /// - Postcondition: The returned iterator
  /// - Postcondition: The `Pointee`s at `self[startIndex..<initializedUpTo]`
  /// are initialized.

It looks like the first postcondition got cut off.

···

On Dec 7, 2016, at 10:07 PM, Douglas Gregor <dgregor@apple.com> wrote:

Hello Swift community,

The review of SE-0147 "Move UnsafeMutablePointer.initialize(from:) to UnsafeMutableBufferPointer" begins now and runs through December 12, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.md


(Andrew Trick) #3

For UnsafeMutableRawBufferPointer.initializeMemory:

We have this doc comment:

  /// Returns an iterator to any elements of `source` that didn't fit in the
  /// buffer, and an index into the buffer one past the last byte written.

Which is consistent with the PR https://github.com/apple/swift/pull/5718

+ public func initializeMemory<S: Sequence>(
+ as: S.Iterator.Element.Type, from source: S
+ ) -> (unwritten: S.Iterator, initializedUpTo: Index) {

However, the proposal reads:

public func initializeMemory<S: Sequence>(
     as: S.Iterator.Element.Type, from source: S
  ) -> (unwritten: S.Iterator, initialized: UnsafeMutableBufferPointer<S.Iterator.Element>)

Which API did we end up deciding on?

(unwritten:, initialized:) makes sense to me, but I can’t remember if we ditched that approach for some reason (e.g. consistency with UnsafeMutableBufferPointer).

-Andy

···

On Dec 7, 2016, at 10:07 PM, Douglas Gregor via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0147 "Move UnsafeMutablePointer.initialize(from:) to UnsafeMutableBufferPointer" begins now and runs through December 12, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.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. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.md


(Ben Cohen) #4

Thanks Alex, sorry about that. This should probably read “The returned iterator will yield any remaining elements of the sequence not written to the buffer”. I’m not sure if this really needs to be a postcondition though, in addition to being the documented behavior of the return value.

···

On Dec 8, 2016, at 10:24 AM, Alex Martini via swift-evolution <swift-evolution@swift.org> wrote:

On Dec 7, 2016, at 10:07 PM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

Hello Swift community,

The review of SE-0147 "Move UnsafeMutablePointer.initialize(from:) to UnsafeMutableBufferPointer" begins now and runs through December 12, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.md

extension UnsafeMutableBufferPointer {
  /// Initializes memory in the buffer with the elements of `source`.
  /// Returns an iterator to any elements of `source` that didn't fit in the
  /// buffer, and an index to the point in the buffer one past the last element
  /// written (so `startIndex` if no elements written, `endIndex` if the buffer
  /// was completely filled).
  ///
  /// - Precondition: The memory in `self` is uninitialized. The buffer must contain
  /// sufficient uninitialized memory to accommodate `source.underestimatedCount`.
  ///
  /// - Postcondition: The returned iterator
  /// - Postcondition: The `Pointee`s at `self[startIndex..<initializedUpTo]`
  /// are initialized.

It looks like the first postcondition got cut off.
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Ben Cohen) #5

The proposed API in the proposal is right, the comment above it should have been updated. The intention is for the raw buffer version to return an initialized typed buffer not an index into the raw buffer. While this means it’s inconsistent with the typed equivalent, an index into the raw buffer isn’t all that useful as usually you want to operate on typed values after you’ve initialized them, and the calculation to get to the raw buffer index from the typed buffer if you need it is easy i.e. initialized.count * the stride of the type.

(I haven’t yet updated the PR to reflect this change yet either, was waiting to see if there was more feedback first to incorporate)

···

On Dec 12, 2016, at 1:32 PM, Andrew Trick <atrick@apple.com> wrote:

On Dec 7, 2016, at 10:07 PM, Douglas Gregor via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hello Swift community,

The review of SE-0147 "Move UnsafeMutablePointer.initialize(from:) to UnsafeMutableBufferPointer" begins now and runs through December 12, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.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. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.md

For UnsafeMutableRawBufferPointer.initializeMemory:

We have this doc comment:

  /// Returns an iterator to any elements of `source` that didn't fit in the
  /// buffer, and an index into the buffer one past the last byte written.

Which is consistent with the PR https://github.com/apple/swift/pull/5718

+ public func initializeMemory<S: Sequence>(
+ as: S.Iterator.Element.Type, from source: S
+ ) -> (unwritten: S.Iterator, initializedUpTo: Index) {

However, the proposal reads:

public func initializeMemory<S: Sequence>(
     as: S.Iterator.Element.Type, from source: S
  ) -> (unwritten: S.Iterator, initialized: UnsafeMutableBufferPointer<S.Iterator.Element>)

Which API did we end up deciding on?

(unwritten:, initialized:) makes sense to me, but I can’t remember if we ditched that approach for some reason (e.g. consistency with UnsafeMutableBufferPointer).

-Andy


(Andrew Trick) #6

That’s good. Those are the reasons the proposed API makes more sense to me too.

-Andy

···

On Dec 12, 2016, at 6:30 PM, Ben Cohen <ben_cohen@apple.com> wrote:

On Dec 12, 2016, at 1:32 PM, Andrew Trick <atrick@apple.com <mailto:atrick@apple.com>> wrote:

On Dec 7, 2016, at 10:07 PM, Douglas Gregor via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hello Swift community,

The review of SE-0147 "Move UnsafeMutablePointer.initialize(from:) to UnsafeMutableBufferPointer" begins now and runs through December 12, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.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. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0147-move-unsafe-initialize-from.md

For UnsafeMutableRawBufferPointer.initializeMemory:

We have this doc comment:

  /// Returns an iterator to any elements of `source` that didn't fit in the
  /// buffer, and an index into the buffer one past the last byte written.

Which is consistent with the PR https://github.com/apple/swift/pull/5718

+ public func initializeMemory<S: Sequence>(
+ as: S.Iterator.Element.Type, from source: S
+ ) -> (unwritten: S.Iterator, initializedUpTo: Index) {

However, the proposal reads:

public func initializeMemory<S: Sequence>(
     as: S.Iterator.Element.Type, from source: S
  ) -> (unwritten: S.Iterator, initialized: UnsafeMutableBufferPointer<S.Iterator.Element>)

Which API did we end up deciding on?

(unwritten:, initialized:) makes sense to me, but I can’t remember if we ditched that approach for some reason (e.g. consistency with UnsafeMutableBufferPointer).

-Andy

The proposed API in the proposal is right, the comment above it should have been updated. The intention is for the raw buffer version to return an initialized typed buffer not an index into the raw buffer. While this means it’s inconsistent with the typed equivalent, an index into the raw buffer isn’t all that useful as usually you want to operate on typed values after you’ve initialized them, and the calculation to get to the raw buffer index from the typed buffer if you need it is easy i.e. initialized.count * the stride of the type.

(I haven’t yet updated the PR to reflect this change yet either, was waiting to see if there was more feedback first to incorporate)