[swift-evolution] [Review] SE-0184: Unsafe[Mutable][Raw][Buffer]Pointer: add missing methods, adjust existing labels for clarity, and remove deallocation size

[Taylor Swift]

On Sat, Sep 2, 2017 at 3:39 PM, Xiaodi Wu <xiaodi.wu at gmail.com> wrote:

> On Sat, Sep 2, 2017 at 2:13 PM, Taylor Swift <kelvin13ma at gmail.com> wrote:
>
>>
>>
>> On Sat, Sep 2, 2017 at 10:03 AM, Xiaodi Wu via swift-evolution <
>> swift-evolution at swift.org> wrote:
>>
>>> On Sat, Sep 2, 2017 at 12:27 AM, Douglas Gregor via swift-evolution <
>>> swift-evolution at swift.org> wrote:
>>>
>>>> Hello Swift community,
>>>>
>>>> The review of SE-0184 "Unsafe[Mutable][Raw][Buffer]Pointer: add
>>>> missing methods, adjust existing labels for clarity, and remove
>>>> deallocation size" begins now and runs through September 7, 2017. The
>>>> proposal is available here:
>>>>
>>>> https://github.com/apple/swift-evolution/blob/master/proposa
>>>> ls/0184-unsafe-pointers-add-missing.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/proposa
>>>> ls/0184-unsafe-pointers-add-missing.md
>>>>
>>>> Reply text
>>>>
>>>> Other replies
>>>>
>>>>
>>>> <https://github.com/apple/swift-evolution/pulls#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?
>>>>
>>>> Overall, this is an improvement. However, I do have some questions and
>>> concerns:
>>>
>>>
>>> Questions:
>>>
>>> ## UnsafeMutableRawPointer
>>>
>>> Regarding the options given for "whose `count` to use"--which option is
>>> actually being proposed?
>>>
>>
>> I don’t understand the question,, `UnsafeMutableRawPointer` takes an
>> explicit `count:`. the “whose count to use” option is irrelevant.
>>
>
> In "Proposed Solution," under subheading "UnsafeMutableRawPointer," you
> write "the question of whose `count` to use becomes important." You then
> outline "[o]ne option" as well as "[a] better option." Which of these
> options are you actually proposing? For clarity, could you please excise
> the non-proposed option from the "Proposed Solution" section and move it to
> the "Alternatives Considered" section?
>

The *Proposed solution* section is divided into two parts, one dealing with
plain pointers and one dealing with buffer pointers. The sections are
separated by the horizontal rule above “*Buffer pointers are conceptually
similar…*”. I don’t know why we’re arguing over typographic formatting but
I am glad this proposal is noncontroversial enough to induce debate over
its horizontal rules. As for the two options, the first is a strawman to
explain why we are going with the second option.


>
> ## UnsafeMutableBufferPointer
>>>
>>> Please clarify: why are you proposing that the `at:` arguments in
>>> `UnsafeMutableBufferPointer` and `UnsafeMutableRawBufferPointer`
>>> _should not_ receive default values, but the `at:` arguments in
>>> `UnsafeMutableRawPointer` _should_ receive a default value of `0`?
>>>
>>>
>>  The extant API for `UnsafeMutableRawPointer` already included these
>> default arguments which seem to be widely used in the stdlib,, the proposal
>> tries to avoid these kinds of source breakages wherever possible. We avoid
>> providing the default arguments on buffer pointers because we want the fact
>> that it takes a *start–length* segment pair to be obvious at the call
>> site.
>>
>
> Thanks for the clarification; that would be helpful information to put
> into the proposal text. It is not an intuitive start-length pair, since the
> `at` refers to an offset of the destination buffer but `count` refers to a
> length of the source buffer. I appreciate how you separated the proposed
> new argument `at` and the existing argument `count` in what is currently
> named `initializeMemory<T>(as:from:count:)`, which helps to reinforce
> that fact.
>
>
>> Concerns:
>>>
>>> ## UnsafeMutablePointer
>>>
>>> It's alarming that omitting `count` in `initialize(repeating:count:)`
>>> (and assign, etc.) means initialize _one_ element, but elsewhere (such as
>>> `UnsafeMutableBufferPointer` means initialize _all_ elements. The behavior
>>> of the proposed API also contradicts its own spelling on its face:
>>> `initialize(repeating: foo)` means *do not repeat* `foo`.
>>>
>>> Yes, I understand the argument that `*BufferPointer` types have an
>>> intrinsic count, etc., but in the context of code where types are inferred,
>>> `let foo = T.allocate(capacity: 100); foo.initialize(repeating: bar)`
>>> should not mean one thing for `*BufferPointer` types and a totally
>>> different thing for plain `*Pointer` types--particularly when both can be
>>> allocated with a certain capacity greater than one.
>>>
>>> Either `count` should always be required, or for convenience there
>>> should be a separate overload `initialize(pointee:)` that does not require
>>> `count`.
>>>
>>>
>> I understand the naming is not optimal, but reams of discussion on this
>> list have concluded that it’s the least bad alternative available. We can’t
>> just get rid of the default value for `count:` because usage in real
>> code bases shows that this default argument is actually extremely useful. I
>> believe well over 90% of the calls to these methods in the standard library
>> currently rely on the default argument value. Renaming the `repeating:`
>> argument to `to:` would make the API inconsistent and hide the fact that
>> plain pointers are still capable of operating on many elements in sequence
>> — “`to:count:`” makes no grammatical sense to read — “to” is a singular
>> preposition.
>>
>
> Let me clarify my concern here. This is not intended to be a bikeshedding
> exercise and I agree with your choice of `repeating` (as I did in the
> original conversations). For the sake of clarity, however, I'll proceed
> with this discussion as though the argument were named `xxxx`. My point
> here is that, regardless of what `xxxx` is called, we have a problem here
> as follows:
>
> ```
> let foo = UnsafeMutablePointer<Int>.allocate(capacity: 21)
> foo.initialize(xxxx: 42) // initializes 1 value
>
> let bar = UnsafeMutableBufferPointer<Int>.allocate(capacity: 21)
> bar.initialize(xxxx: 42) // initializes 21 values
> ```
>
> The same spelling, `initialize(xxxx:)`, does two *different* things under
> your proposal depending on whether it's invoked on a UMP or a UMBP. Even
> though it's admirable that your proposal is filling in the gaps of Swift's
> pointer design and increasing consistency by making similar things have
> similar names, different things need to have different names; otherwise, we
> are actively creating footguns.
>
> Therefore, while I agree with your choice for `xxxx`, and while I also
> agree that it is very useful to have a method that initializes a single
> value on a UMP, we need to have a different label `yyyy` for that method.
> My suggestion is `pointee`, but I would be equally happy with `value`,
> `to`, or whatever else you may choose.
>

yes this is a problem, but your solution is to add a second set of methods
that are functionally identical, except for the names of the argument
labels. This strikes me as silly and wasteful of API surface.


>
> ## UnsafeMutableRawBufferPointer
>>>
>>> In `copyBytes`, the use of `Bytes` to emphasize that it's the memory
>>> that's being copied is thoughtful, but it is inconsistent with the other
>>> method names that use the terminology `Memory` for the same purpose (e.g.,
>>> `moveInitializeMemory` to clarify the meaning of `moveInitialize`).
>>>
>>> For better consistency--and since you're proposing to rename
>>> `copyBytes(from:count:)` on `UnsafeMutableRawPointer` to
>>> `copy(from:bytes:)`--this particular API on `UnsafeMutableRawBufferPointer`
>>> should be named `copyMemory(from:)` and not `copyBytes(from:)`.
>>>
>>> Although, actually, looking at the APIs on `UnsafeMutableRawPointer`
>>> itself, that particular method too might best be written as
>>> `copyMemory(from:bytes:)` instead of merely `copy(from:bytes:)` for better
>>> consistency with the rest of the methods on that type as well.
>>>
>>>
>>>
>> “Memory” methods are distinct from “Bytes” methods in that they assume
>> typed memory. “Bytes” on the other hand does not care about types.
>>
>
> It's unclear to me that this distinction is either consistently observed
> or helpful. For instance, by that standard, `initializeMemory(as:from:)`
> should be named `initializeBytes(as:fromMemory:)`, since the memory being
> initialized is raw until after initialization. This strikes me as not at
> all necessary for user comprehension of the APIs. (On the other hand, if it
> _is_ necessary for comprehension, then `copyBytes` should never be
> shortened to `copy`, since it would be necessary to emphasize that both the
> source and destination are treated as "bytes" rather than "memory.")
>

`copyMemory` would probably be better and I am not opposed to adding it to
the proposal. at the same time I don’t think it’s so bad a problem that it
really *needs* to be renamed.


>
> ## General comment
>>>
>>> Many `at:` arguments, especially such as in the case of
>>> `copyBytes(at:from:)`, make sense only when read in a list with all other
>>> methods. Standing alone, `at` is ambiguous as to whether it's referring to
>>> the _source_ or the _destination_. Why do these APIs on `*BufferPointer`
>>> types not take advantage of subscripts? That is, why not:
>>>
>>>   `foo[x...].copyMemory(from: bar)`
>>>
>>> instead of
>>>
>>>   `foo.copyBytes(at: x, from: bar)`
>>>
>>> The first seems dramatically clearer as to its meaning. The same
>>> feedback applies to nearly all uses of `at` on `*BufferPointer` types: they
>>> would seem to be greatly clarified (in terms of the "what does `at` mean"
>>> question) by the use of a subscript spelling.
>>>
>>
>> This idea sounds elegant on the surface but it’s deeply problematic. `
>> foo[x...]` suggests that whatever happens to it, will happen to the
>> entire rest of the buffer slice as well, which is not always the case.
>>
>
> No more so than `foo.copyBytes(at:from:)` suggests the same?
>

`at:` suggests an unbounded starting point. `x...` suggests a concrete
range of the buffer since it’s really shorthand for `x ... endIndex`.


>
>
>> It would have to be written as `foo[x ... x + count].copyMemory(from:
>> bar)` or `foo[x ... x + bar.count].copyMemory(from: bar)` which seems
>> *less* clear.  Having to write `foo[0...]` to operate with no offset
>> also seems silly.
>>
>
> It is unclear to me why one would have to write `foo[0...]`.
>

How else would you operate at offset 0?


>
>
>> It also means the API would have to be written on `
>> MutableRandomAccessSlice<UnsafeMutable*BufferPointer>`.
>>
>
> Not necessarily; it's unclear to me that `MutableRandomAccessSlice<UnsafeMutable*BufferPointer>`
> is the right slice type for `UnsafeMutable*BufferPointer` in the first
> place (see below).
>
>
>> Finally, what happens when you subscript a raw pointer?
>>
>
> As you know, only buffer pointers conform to `Collection`, so it's unclear
> to me what your question is here.
>

I meant a raw *buffer* pointer.


>
>
>> the subscript doesn’t know about the stride that you want to use. If you
>> want to get rid of the `at:` ambiguity, you have to get rid of it
>> everywhere, or users will just wind up having to remember two ways (one
>> ambiguous and one less ambiguous) of doing the same thing, instead of one
>> (ambiguous) way of doing it.
>>
>
> Certainly, that's a good point. On rethink and another re-reading of the
> proposal, it's unclear to me that the addition of `at` arguments to
> UnsafeMutablePointer is sufficiently justified by the proposal text. Is it
> merely that it's shorter than writing `foo + MemoryLayout<T>.stride *
> offset`? With the ambiguity of `at`, it seems that the current way of
> writing it, though longer, is certainly less ambiguous.
>

Please reread it; UnsafeMutablePointer’s methods do *not* use `at:`.


>
> I notice that you comment that you feel there are ergonomic issues with
>>> buffer pointer slicing; can you please comment on what is "wasteful"
>>> currently? Is there a performance hit to slicing a `*BufferPointer` type?
>>> If so, we should fix that, as the whole rationale of having these types (as
>>> I understand it) is to improve the ergonomics of working with pointers to
>>> multiple elements by conforming them to `*Collection` APIs.
>>>
>>
>> Slices are not the right abstraction for this because of Swift’s indexing
>> semantics.
>>
>
> This is an alarming statement if true, in that it would seem to undermine
> the basic premise of `*BufferPointer` types conforming to `Collection`,
> would it not?
>

I think Andy’s message has a good explanation for this.


>
>
>> A buffer pointer requires two words of information (beginning and end
>> address), while a buffer pointer slice occupies four words of information
>> (beginning, end, start-index, end-index) to preserve the index semantics.
>> Creating a way to “slice” a buffer pointer into another buffer pointer
>> without going through `init(rebasing:)` is also problematic because you
>> can’t `deallocate()` a debased buffer pointer, so we should make that
>> operation explicit.
>>
>
> It would seem, then, that to properly support slicing--which collection
> types should do--we will need a custom slice type a la `Substring`. This
> slice type would clearly not support deallocation, but it would conform to
> a protocol (a la `StringProtocol`) which would require all the operations
> such as `copyMemory` that makes sense for both buffer pointers and their
> slices.
>

You’re basically proposing Protocol Oriented Pointers. Probably a good
eventual goal, but we need to take things step by step.


>
> It seems deeply unsatisfactory to invent new methods that use `at:`
>>> arguments _on a type whose purpose is to expose `*Collection` APIs_ if we
>>> agree that the slicing notation is demonstrably clearer. I do not have the
>>> same concerns about adding `at:` arguments to `UnsafeMutableRawPointer`
>>> methods, which are quite appropriate.
>>>
>>>
>>>
>>>>    - Is the problem being addressed significant enough to warrant a
>>>>    change to Swift?
>>>>
>>>> Yes.
>>>
>>>
>>>>
>>>>    - Does this proposal fit well with the feel and direction of Swift?
>>>>
>>>> In parts, yes. In others (see above), it could be improved to fit
>>> better with the feel and direction of other Swift APIs.
>>>
>>>
>>>>
>>>>    - If you have used other languages or libraries with a similar
>>>>    feature, how do you feel that this proposal compares to those?
>>>>
>>>> There is much more friction to using pointers in Swift than in C.
>>> However, making Swift pointers like C pointers is clearly a non-goal. This
>>> proposal appropriate addresses major pain points to Swift-specific usages
>>> of pointers.
>>>
>>>
>>>>
>>>>    - How much effort did you put into your review? A glance, a quick
>>>>    reading, or an in-depth study?
>>>>
>>>> A moderately thorough reading, drawing on some experience using pointer
>>> APIs in Swift, and based on prior readings of previous iterations of this
>>> proposal and the on-list discussion.
>>>
>>>
>>>> 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
>>>>
>>>> _______________________________________________
>>>> swift-evolution mailing list
>>>> swift-evolution at swift.org
>>>> https://lists.swift.org/mailman/listinfo/swift-evolution
>>>>
>>>>
>>>
>>> _______________________________________________
>>> swift-evolution mailing list
>>> swift-evolution at swift.org
>>> https://lists.swift.org/mailman/listinfo/swift-evolution
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170902/3d0a09c6/attachment.html>