Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I am, however, disappointed by that inout parameter to the function withUnsafeMutableBufferPointer. The best way to improve the ergonomics of the Mutable*BufferPointer types with respect to MutableCollection would be to give them a nonmutating interface to MutableCollection. Then, we could have a constant parameter, and implementations wouldn't need to check for buffer identity, as it would be enforced by the compiler.
+1. I think this is a valuable addition to the Collection protocol hierarchy. We've been talking about this protocol for years - I'm thrilled to see it finally come up for inclusion.
Is the problem being addressed significant enough to warrant a change to Swift?
Yes.
There is a clear performance benefit to knowing that a Collection's elements are stored contiguously, especially if its implementation/iterator are otherwise opaque.
Since C arrays are pointers, interop with C libraries becomes much easier.
We have several types in the standard library, corelibs and associated projects which already use this pattern to provide a pointer to their storage. Library authors would like the option to write generic algorithms which care about data rather than some specific semantics implied by a type (e.g. Array vs Data).
Does this proposal fit well with the feel and direction of Swift?
I think so. Although there are a couple of nits:
If we can add a non-mutating MutableCollection conformance to UMBP (as suggested above), that would clearly be preferable to an inout parameter.
I prefer the name withUnsafeMutableBufferPointerIfContiguous, because I think it more clearly implies that this is a kind of type narrowing. That is to say, I think it makes it more obvious that rather than implementing the customisation point directly, you should conform to MutableContiguousCollection. The documentation should also mention this.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
C++17 includes contiguous iterators. As in this proposal, they imply random access.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Read proposal, participated in pitch discussion as well as several others.
However, we have been investigating integration of Data, String, and Sequence in this pull request:
and it does not seem like we have quite what we need yet in this proposal to get the performance benefit we're looking for. At least not yet.
We have had some discussions about potential paths forward, but the current opinion of the Foundation team is that we need some more time to make sure this enough of the right direction before committing to the proposal as written.
I'd much rather have to occasionally add a temporary var than have the misleading function signature. The standard library can ease the pain with non-mutating calls to its algorithms.
The Core Team felt that the use of inout for the closure parameter of withUnsafeMutableBufferPointer(IfSupported) was the best available option, and noted that the library could verify that the UnsafeMutableBufferPointer itself wasn't directly modified by checking for the expected base address / count after the call. This limits the chance of confusion without resorting to shadowing or excessive overloading of mutating operations.
The two proposed protocols (ContiguousCollection and MutableContiguousCollection) aren't used in any algorithms within the library. It is not clear that they are important enough to introduce as protocols into the Standard Library at this time. The Core Team would like to consider a revised proposal that does not introduce these protocols.
There are use cases for a Sequence equivalent to the proposed withUnsafeMutableBufferPointerIfSupported, such as initializing a String from a Sequence of UTF-8 code points that are (e.g.) stored in a Data. The Core Team would like to see this addition to the Sequence protocol, which would allow Sequence clients to optimize for the contiguously-stored case without requiring a new protocol, much as the proposal already allows MutableCollection clients to optimize for the contiguously-stored mutable case.
I'll initiate a new review thread for the revised proposal shortly.