nonmutating swapAt implementations for UnsafeMutable(Raw)BufferPointer #13071
Conversation
| @@ -492,6 +492,29 @@ public struct Unsafe${Mutable}RawBufferPointer | |||
| % end # mutable | |||
| } | |||
|
|
|||
| %if Mutable: | |||
| /// Exchanges the byte values at the specified offsets | |||
| /// in the memory region | |||
There was a problem hiding this comment.
Can we have a period (.) at the end of this sentence.
| /// Exchanges the byte values at the specified offsets | ||
| /// in the memory region | ||
| /// | ||
| /// Both parameters must be valid indices, and not |
There was a problem hiding this comment.
...valid indices in the memory region?
| let pi = (_position! + i) | ||
| let pj = (_position! + j) | ||
| let tmp = pi.load(fromByteOffset: 0, as: UInt8.self) | ||
| pi.copyBytes(from: pj, count: MemoryLayout<UInt8>.size) |
There was a problem hiding this comment.
Why not count: 1? If UInt8 is not one byte, then you've broken the semantic guarantees of the method anyway.
There was a problem hiding this comment.
Why not count: 1?
True. Using literal 1 would me more consistent with the pointer arithmetic above.
There was a problem hiding this comment.
I think using an explicitly typed value from MemoryLayout is more consistent with the load and storeBytes methods, which must be typed.
| @@ -492,6 +492,29 @@ public struct Unsafe${Mutable}RawBufferPointer | |||
| % end # mutable | |||
| } | |||
|
|
|||
| %if Mutable: | |||
| /// Exchanges the byte values at the specified offsets | |||
| /// in the memory region. | |||
There was a problem hiding this comment.
Question: What is the difference in meaning between this “in the memory region” and the documentation for the other method which says, “of the buffer”?
There was a problem hiding this comment.
"memory region" is used regularly in the UnsafeRawPointer documentation, which I interpret as emphasis on the untyped nature of raw pointers; I didn't use it well here.
glessard
force-pushed
the
umbp-nonmutating-swapat
branch
from
b1aae41 to
72f1dac
Compare
|
@swift-ci please test |
glessard
force-pushed
the
umbp-nonmutating-swapat
branch
4 times, most recently
from
b2c2769 to
a0722b2
Compare
|
@atrick I realized that one thing these implementations lose is any debug-mode bounds checking. (The default implementation gets it for free by using the subscript). Would it be better to add some |
|
Thanks. |
glessard
force-pushed
the
umbp-nonmutating-swapat
branch
2 times, most recently
from
13b3ab4 to
829977f
Compare
glessard
force-pushed
the
umbp-nonmutating-swapat
branch
from
829977f to
0c95f90
Compare
|
@atrick Sorry to bother again, but this seems to have fallen off everyone's radar… |
|
@swift-ci test. |
|
Thanks! |
|
@glessard thanks for bring this to my attention again. It should have been merged a long time ago! |
|
I thought for a moment it was waiting for 4.1, but then it didn't look like it! |
|
@glessard We need a motivating benchmark for a change like that. If a motivating benchmark exists then it's certainly relevant. |
|
@atrick Is it sufficient to start a PR and benchmark it from there? I imagine that if the change is not positive then it would equally justify removing the specialized forms from |
|
Hopefully there's already a benchmark that relies on the non-raw pointer specialization. I mean that a PR for raw pointer specialization needs to include such a benchmark. I suspect that we don't already have one. The benchmarks only cover things that have already been deliberately optimized. |
|
Now I see what you meant. I'll see what I can do, then. |
The
BufferPointertypes currently useMutableCollection's default implementation ofswapAt, which is unnecessarily mutable and, in the case ofUnsafeMutableBufferPointerdoes nothing to minimize ARC traffic. This PR introduces non-mutating implementations ofswapAtforUnsafeMutableBufferPointerandUnsafeMutableRawBufferPointer, in the spirit of #12504. If this gets merged some new "un-mutated var" warnings may appear, but source code would otherwise be unaffected.