[swift-evolution] SE-184 Improved Pointers

[Taylor Swift]

done: https://github.com/apple/swift-evolution/pull/744

On Wed, Aug 23, 2017 at 1:56 PM, Andrew Trick <atrick at apple.com> wrote:

> Kelvin,
>
> Please resubmit a clean swift-evolution PR now. I personally think this is
> ready for formal review given that all feedback was positive and all issues
> brought up during review have been addressed.
>
> -Andy
>
> On Aug 22, 2017, at 12:59 PM, Michael Ilseman <milseman at apple.com> wrote:
>
> This is an excellent, thoroughly thought out, and well written proposal!
> I’m eager to see these improvements land.
>
>
> On Aug 22, 2017, at 11:33 AM, Taylor Swift <kelvin13ma at gmail.com> wrote:
>
>
>
> On Tue, Aug 22, 2017 at 2:35 AM, Andrew Trick <atrick at apple.com> wrote:
>
>>
>> On Aug 21, 2017, at 10:59 PM, Taylor Swift <kelvin13ma at gmail.com> wrote:
>>
>> Sorry to bring this up again, but I was not able to defend the addition
>>> of `UnsafeMutableBufferPointer.de
>>> <http://unsafemutablebufferpointer.de/>initialize()`. It is incorrect
>>> for the typical use case and doesn't appear to solve any important use
>>> case. The *only* fully initializing method is `initialize(repeating:)`, but
>>> that will usually be used for trivial values, which should not be
>>> deinitialized. It's preferable for the user to explicitly deinitialize just
>>> the segments that they know were initialized, which can be done on the base
>>> pointer. The only benefit in having a `deinitialize` on the buffer is to
>>> communicate to users who see the `initialize` API for the first time that
>>> it is their responsibility to deinitialize if the type requires it. To that
>>> end, we could add a `deinitialize(at:count:)` method, communicating the
>>> symmetry with `initialize(at:from:). Naturally `index + count <=
>>> self.count`.
>>>
>>> -Andy
>>>
>>
>> I don’t agree with this. If `deinitialize()` is a problem because it
>> deinitializes the entire buffer, so are `moveAssign` and `moveInitialize`.
>> They all assume the released buffer operand is fully initialized. `
>> deinitialize()` has just as much use as the other full-buffer releasing
>> methods. Just take the image buffer example there
>>
>>
>> `moveAssign` and `moveInitialize` assume that the sub-buffer being moved
>> from is fully initialized. That’s already obvious because the user is
>> asking to move source.count elements. I don’t see any use cases where it
>> would pose a problem. If the user is moving out of a partially initialized
>> buffer, they have already to sliced (and unfortunately rebased) the buffer.
>> OTOH `deinitialize` is incorrect for normal use cases. I don’t see any
>> practical analogy between those APIs.
>>
>> let pixels:Int = scanlines.map{ $0.count }.reduce(0, +)var image = UnsafeMutableBufferPointer<Pixel>.allocate(capacity: pixels)
>> var filled:Int = 0for scanline:UnsafeMutableBufferPointer<Pixel> in scanlines
>> {
>>     image.moveInitialize(at: filled, from: scanline)
>>     filled += scanline.count
>> }
>>
>> image.deinitialize()
>>
>> We don’t want developers to do this. Instead we want to see an explicitly
>> named association between the number of items initialized and deinitialized:
>>
>> image.deinitialize(at: 0, count: filled)
>>
>> Flipping this around, it could be even more common to be writing into a
>> larger than necessary buffer (pixels > filled). If we’re providing
>> auto-slicing initializers, then deinitialization should follow the same
>> approach, rather than:
>>
>> UnsafeMutableBufferPointer(rebasing: image[0, filled]).deinitialize()
>>
>> image.deallocate()
>>
>> and replace `Pixel` with a class type like `UIButton`.
>>
>> And `deinitialize(at:count:)` is bad because you’re asking for a count
>> on a buffer method. `moveAssign` and `moveInitialize` can take range
>> parameters because they each have a second operand that supplies the count
>> number. `deinitialize` doesn’t. That means calls could end up looking
>> like
>>
>> buffer.deinitialize(at: 0, count: buffer.count)
>>
>> which is exactly what we were trying to avoid in the first place.
>>
>>
>> But there is no value in avoiding the `count` argument here. That’s not a
>> valid motivation for introducing `deinitialize` on a buffer, and we’d be
>> better off not introducing it at all.
>>
>> The only valid motivation I can come up with for introducing
>> `deinitialize` on buffer is to remind developers who are only looking at
>> the buffer API (and not the plain pointer API) that it’s their
>> responsibility to manually deinitialize (it doesn’t automatically happen on
>> deallocation or destruction).
>>
>> -Andy
>>
>>
> I replaced UnsafeMutableBufferPointer.deinitialize() with
> UnsafeMutableBufferPointer.deinitialize(at:count:)
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170823/b946a43c/attachment.html>