<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 21, 2017 at 11:09 PM, Andrew Trick <span dir="ltr"><<a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><span class="gmail-"><br><div><blockquote type="cite"><div>On Aug 20, 2017, at 6:03 PM, Taylor Swift <<a href="mailto:kelvin13ma@gmail.com" target="_blank">kelvin13ma@gmail.com</a>> wrote:</div><br class="gmail-m_8345471542714458613Apple-interchange-newline"><div><div dir="ltr"><div>New draft of the proposal is up here: <<a href="https://github.com/kelvin13/swift-evolution/blob/patch-3/proposals/0184-improved-pointers.md" target="_blank">https://github.com/kelvin13/<wbr>swift-evolution/blob/patch-3/<wbr>proposals/0184-improved-<wbr>pointers.md</a>><br><br></div>Important changes <a href="https://github.com/kelvin13/swift-evolution/blob/patch-3/proposals/0184-improved-pointers.md#proposed-solution" target="_blank">start here</a>.</div></div></blockquote><br></div></span><div>I have more feedback for you after discussing with Ben and Michael.</div><div><br></div><div><div>Under the section "Background". The retaining/releasing counts are confusing because we normally talk about "+0/+1" with respect to a single value. Here you're summing the reference count effect across different source and destination values. I think you can drop the -1/+0/+1 from the cell labels and just label the headers as "Source-Copy(+1) vs. Source-Move(+0)" and "Dest-Initializing(+0)" vs. "Dest-Destroying(-1)".</div></div></div></blockquote><div><br></div><div>I got rid of the total counts, but I used “retaining” and “releasing” because the word “initialize” is already way overloaded in the document and using it as a category in addition to referring to the actual method family `<span style="font-family:monospace,monospace">initialize</span>` and `<span style="font-family:monospace,monospace">initializeMemory</span>` and Swift’s `<span style="font-family:monospace,monospace">init</span>` initializers would just get too confusing.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div><br></div><div>Ben noticed that we still don't have `UnsafeMutableBufferPointer.<wbr>deallocate()`. Please add that as well.</div></div></div></blockquote><div><br></div><div>This is in the implementation and 2/3 of the API mocks; idk why it was lost in the detailed changes section. That was a typo I just fixed it<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div>I think you should also add the default `alignedTo:Int = MemoryLayout<UInt>.alignment` to `UnsafeRawMutablePointer`. It is effectively part of Swift's memory model... [Background: We're providing a language level default guarantee of word-aligned storage. We don't want to expose the platform's default alignment. There is no such thing as a "maximal" alignment for all imported primitive types. We expect allocators to typically provide 16-byte alignment, but when developers are relying on that for correctness, we want it to be explicit. Developers usually rely on word alignment so there's no value in making that explicit.]</div></div></div></blockquote><div><br></div><div>done and committed</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div><br></div><div>The source breaking changes can be marked deprecated for now, but can only be marked unavailable in Swift 5 mode or later.</div></div></div></blockquote><div><br></div><div>done and committed<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div><br></div><div>We anticipate adding a ContiguouslyStored protocol "soon", which could be used to reduce the amount of overloading on the buffer `initialize`/`assign` APIs. But we need a lot more time to iterate on that design and it doesn't appear that migrating to it would be source breaking w.r.t. your proposal.</div><div><br></div><div>I think we would be receptive to a "non-Optional baseAddress" proposal now if you or anyone else wants to pitch that. I know Dave Abrahams had a working implementation at some point. That would weaken some of the incentive to continue adding more convenience to buffer slices.</div></div></div></blockquote><div><br></div><div>Really early versions of this proposal pitched that but it didn’t go so well. also making baseAddress nonnillable means you don’t always have access to `count` when `count == 0`<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div><br></div><div>Sorry to bring this up again, but I was not able to defend the addition of `UnsafeMutableBufferPointer.<wbr>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`.</div><div><br></div><div>-Andy</div></div></div></blockquote><div><br></div><div>I don’t agree with this. If `<span style="font-family:monospace,monospace">deinitialize()</span>` is a problem because it deinitializes the entire buffer, so are `<span style="font-family:monospace,monospace">moveAssign</span>` and `<span style="font-family:monospace,monospace">moveInitialize</span>`. They all assume the released buffer operand is fully initialized. `<span style="font-family:monospace,monospace">deinitialize()</span>` has just as much use as the other full-buffer releasing methods. Just take the image buffer example there <br></div><div><br></div><div><pre><span class="gmail-pl-k">let</span> pixels<span class="gmail-pl-k">:</span><span class="gmail-pl-c1">Int</span> <span class="gmail-pl-k">=</span> scanlines.<span class="gmail-pl-c1">map</span>{ <span class="gmail-pl-c1">$0</span>.<span class="gmail-pl-c1">count</span> }.<span class="gmail-pl-c1">reduce</span>(<span class="gmail-pl-c1">0</span>, <span class="gmail-pl-k">+</span>)
<span class="gmail-pl-k">var</span> image <span class="gmail-pl-k">=</span> <span class="gmail-pl-c1">UnsafeMutableBufferPointer</span><span class="gmail-pl-k"><</span>Pixel<span class="gmail-pl-k">></span>.<span class="gmail-pl-c1">allocate</span>(<span class="gmail-pl-c1">capacity</span>: pixels)
<span class="gmail-pl-k">var</span> filled<span class="gmail-pl-k">:</span><span class="gmail-pl-c1">Int</span> <span class="gmail-pl-k">=</span> <span class="gmail-pl-c1">0</span>
<span class="gmail-pl-k">for</span> scanline<span class="gmail-pl-k">:</span><span class="gmail-pl-c1">UnsafeMutableBufferPointer</span><span class="gmail-pl-k"><</span>Pixel<span class="gmail-pl-k">></span> <span class="gmail-pl-k">in</span> scanlines
{
image.<span class="gmail-pl-c1">moveInitialize</span>(<span class="gmail-pl-c1">at</span>: filled, <span class="gmail-pl-c1">from</span>: scanline)
filled <span class="gmail-pl-k">+=</span> scanline.<span class="gmail-pl-c1">count</span>
}
image.<span class="gmail-pl-c1">deinitialize</span>()
image.<span class="gmail-pl-c1">deallocate</span>()<br><br></pre><pre><span style="font-family:arial,helvetica,sans-serif">and replace `<span style="font-family:monospace,monospace">Pixel</span>` with a class type like `<span style="font-family:monospace,monospace">UIButton</span>`.</span><br></pre></div></div>And `<span style="font-family:monospace,monospace">deinitialize(at:count:)</span>` is bad because you’re asking for a count on a buffer method. `<span style="font-family:monospace,monospace">moveAssign</span>` and `<span style="font-family:monospace,monospace">moveInitialize</span>` can take range parameters because they each have a second operand that supplies the count number. `<span style="font-family:monospace,monospace">deinitialize</span>` doesn’t. That means calls could end up looking like <br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><span style="font-family:monospace,monospace">buffer.deinitialize(at: 0, count: buffer.count)</span></div><div class="gmail_extra"><br></div><div class="gmail_extra">which is exactly what we were trying to avoid in the first place.<br></div></div>