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 <milse...@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 <kelvin1...@gmail.com >> <mailto:kelvin1...@gmail.com>> wrote: >> >> >> >> On Tue, Aug 22, 2017 at 2:35 AM, Andrew Trick <atr...@apple.com >> <mailto:atr...@apple.com>> wrote: >> >>> On Aug 21, 2017, at 10:59 PM, Taylor Swift <kelvin1...@gmail.com >>> <mailto:kelvin1...@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 = 0 >>> for 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:) >
_______________________________________________ swift-evolution mailing list swift-evolution@swift.org https://lists.swift.org/mailman/listinfo/swift-evolution
