On Sat, Sep 2, 2017 at 12:27 AM, Douglas Gregor via swift-evolution < [email protected]> 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/ > proposals/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/ > proposals/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? ## 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`? 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`. ## 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. ## 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. 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. 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 > [email protected] > https://lists.swift.org/mailman/listinfo/swift-evolution > >
_______________________________________________ swift-evolution mailing list [email protected] https://lists.swift.org/mailman/listinfo/swift-evolution
