On Wed, Aug 9, 2017 at 2:34 AM, Andrew Trick <atr...@apple.com> wrote:
> > On Aug 8, 2017, at 11:10 PM, Taylor Swift <kelvin1...@gmail.com> wrote: > > > On Wed, Aug 9, 2017 at 1:51 AM, Andrew Trick <atr...@apple.com> wrote: > >> >> On Aug 8, 2017, at 8:44 PM, Taylor Swift <kelvin1...@gmail.com> wrote: >> >> cool,, as for UnsafeMutableRawBufferPointer.copy(from:bytes:), I cannot >> find such a function anywhere in the API. There is copyBytes(from:) >> <https://developer.apple.com/documentation/swift/unsafemutablerawbufferpointer/2635415-copybytes>, >> but the documentation is messed up and mentions a nonexistent count: >> argument over and over again. The documentation also doesn’t mention what >> happens if there is a length mismatch, so users are effectively relying on >> an implementation detail. I don’t know how to best resolve this. >> >> >> We currently have `UnsafeMutableRawBufferPointer.copyBytes(from:)`. I >> don’t think your proposal changes that. The current docs refer to the >> `source` parameter, which is correct. Docs refer to the parameter name, not >> the label name. So `source.count` is the size of the input. I was pointing >> out that it has the semantics: `debugAssert(source.count <= self.count)`. >> >> Your proposal changes `UnsafeRawPointer.copyBytes(from:count:)` to >> `UnsafeRawPointer.copy(from:bytes:)`. Originally we wanted to those API >> names to match, but I’m fine with your change. What is more important is >> that the semantics are the same as `copyBytes(from:)`. Furthermore, any new >> methods that you add that copy into a raw buffer (e.g. >> initializeMemory(as:from:count:)) should have similar behavior. >> >> > I’m fine with switching to taking the count from the source, though I > think taking the count from the destination is slightly better because 1) > the use cases I mentioned in the other email, and 2) all the other > memorystate functions use self.count instead of source.count, if they > take a source argument. But being consistent with the raw pointer version > is more important. > > > If it’s copying from a buffer it should not take a count, if it’s copying > from a pointer it obviously needs to take a count. What I mean by the two > versions being named consistently is simply that they’re both named > `copyBytes`. That really isn’t important though. The overflow/underflow > semantics being consistent are important. > > (Incidentally, the reason “bytes” needs to be in the somewhere name is > because this method isn’t capable of copying nontrivial values) > > Should the methods that don’t deal with raw buffers also be modified to > use the source argument (i.e. UnsafeMutableBufferPointer. > initialize(from:))? > > > I’m not sure what you mean by this. It also allows the destination to be > larger than the source. Initializing from a sequence does not trap on > overflow because we can’t guarantee the size of the sequence ahead of time. > When I talk about consistent overflow/underflow semantics, I’m only talking > about initializing one unsafe buffer/pointer from another unsafe > buffer/pointer. > > Also, was there a reason why UnsafeMutableRawBufferPointer. > copyBytes(from:) uses the source’s count instead of its own? Right now > this behavior is “technically” undocumented behavior (as the public docs > haven’t been updated) so if there was ever a time to change it, now would > be it. > > > Mainly because partial initialization is more expected than dropping data > on the floor. Ultimately, this should be whatever typical developers would > expect the behavior to be. I would be very hesitant to change the behavior > now though. > > -Andy > The problem is I would expect to be able to safely call deinitialize() and friends after calling initialize(from:). If Element is a class type and initialize doesn’t fill the entire buffer range, calling deinitialize() will crash. That being said, since copy(from:bytes:) and copyBytes(from:) don’t do any initialization and have no direct counterparts in UnsafeMutableBufferPointer, it’s okay if they have different behavior than the other methods. > > — >> >> Another thing. The initialization methods that you’re adding to >> `UnsafeRawPointer` and `UnsafeRawBufferPointer` should return typed >> `UnsafePointer<Element>` and `UnsafeBufferPointer<Element>` respectively. >> > > I’ll fix that once the current pending edit > <https://github.com/apple/swift-evolution/pull/741> gets merged. > > >> >> Thanks, >> >> -Andy >> >> On Tue, Aug 8, 2017 at 11:33 PM, Andrew Trick <atr...@apple.com> wrote: >> >>> >>> On Aug 8, 2017, at 8:29 PM, Taylor Swift <kelvin1...@gmail.com> wrote: >>> >>> >>> >>> On Tue, Aug 8, 2017 at 11:24 PM, Andrew Trick <atr...@apple.com> wrote: >>> >>>> >>>> On Aug 8, 2017, at 6:51 PM, Taylor Swift <kelvin1...@gmail.com> wrote: >>>> >>>> >>>> >>>> On Tue, Aug 8, 2017 at 9:38 PM, Andrew Trick <atr...@apple.com> wrote: >>>> >>>>> >>>>> > UnsafeMutableRawBufferPointer.allocate(bytes:alignedTo:) >>>>>> >>>>>> Well, I think it's somewhat ridiculous for users to write this every >>>>>> time they allocate a buffer: >>>>>> >>>>>> `UnsafeMutableRawBufferPointer.allocate(bytes: size, alignedTo: >>>>>> MemoryLayout<UInt>.alignment)` >>>>>> >>>>>> If anyone reading the code is unsure about the Swift API's alignment >>>>>> guarantee, it's trivial to check the API docs. >>>>>> >>>>>> You could introduce a clearly documented default `alignedTo` >>>>>> argument. The reason I didn't do that is that the runtime won't >>>>>> respect it anyway. But I think it would be fair to go ahead with the >>>>>> API and file a bug against the runtime. >>>>>> >>>>> >>>>> Default argument of MemoryLayout<Int>.alignment is the way to go but >>>>> as you said i don’t know if that is actually allowed/works. An alternative >>>>> is to have two allocate methods each, one that takes an alignment argument >>>>> and one that doesn’t (and aligns to pointer alignment) but that feels >>>>> inelegant. Default arguments would be better. >>>>> >>>>> >>>>> Default argument makes sense to me too. Then the raw buffer pointer >>>>> and regular raw pointer APIs can be consistent with each other. >>>>> >>>>> Runtime bug: https://bugs.swift.org/browse/SR-5664 >>>>> >>>>> >>>> yikes i was not aware of this. I don’t think it’s bad enough to warrant >>>> dropping the argument like with deallocate(capacity:) but I can >>>> imagine bad things happening to code that crams extra inhabitants into >>>> pointers. >>>> >>>> >>>> If we ever need to do pointer adjustment during deallocation to >>>> accommodate alignment, then I think the Swift runtime can track that. I see >>>> no reason to muddy the UnsafeRawPointer API with it. So, I agree with your >>>> proposed change to drop `alignedTo` there. >>>> >>>> -Andy >>>> >>> >>> oh lol I was talking about assuming the pointer returned by >>> allocate(bytes:alignedTo:) is a multiple of alignedTo. Some code might >>> be relying on the last few bits of the pointer being zero; i.e. sticking >>> bit flags there like how some implementations store the red/black color >>> information in a red-black tree node. >>> >>> >>> Oh, sure. But I think it will be easy to fix the runtime. We could >>> probably do it before the proposal is accepted if necessary. >>> -Andy >>> >>> >> >> > >
_______________________________________________ swift-evolution mailing list swift-evolution@swift.org https://lists.swift.org/mailman/listinfo/swift-evolution