[swift-evolution] SE-184 Improved Pointers

[Taylor Swift]

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.deinitialize()`. 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/20170822/05da9421/attachment.html>