remove unnecessary UInt8 pointer binds

[weissi]

CC @atrick who pointed this out in an Swift Evolution post. However I don't believe there are any correctness issues here in ByteBuffer regarding the types as we do always bind the memory to UInt8s when we assumeMemoryBound(to: UInt8.self).

Motivation:

The bytes that a Unsafe(Mutable)RawBufferPointers points to can be
accessed directly as UInt8s. We didn't know that so we bind
ByteBuffers memory to UInt8 so that we can access it as
UnsafePointer<UInt8> using assumingMemoryBound(to: UInt8.self) in
many places. Most of the time that is unnecessarily awkward and
unnecessary.

Modifications:

• remove some easy instances where we access an
  UnsafeMutableBufferPointer as UnsafeBufferPointer<UInt8> just to
  read some UInt8s.

Result:

less awkward looking code

[atrick]

@weissi Wow. Thanks for jumping on this so quickly. I hadn't even gotten around to filing a bug. Your changes look good.

When I refer to "correct" code, it may just mean that I can prove the correctness without understanding much of the source code and that it follows what I consider the intended use of the API.

If you really want to be able to assume that ByteBuffer holds UInt8s, and access it via UnsafePointer, then it would be better to make it a typed pointer of UInt8s. But presumably other types will be stored/loaded here, so UnsafeRawBufferPointer is the right™ type? And if course it can always be viewed as a RandomAccessCollection of UInt8.

If there's something lacking from UnsafeRawBufferPointer that would make your job easier, that would be a great bug report. I know that the UnsafeRawBufferPointer slice (range subscript) API leaves a lot to be desired. We should gather use cases for that.

I'm worried about developers in general thinking that assumingMemoryBound is the intended mechanism for pointer type conversion. It's more of a backdoor for things like the pthread API that takes a void * callback. (This misunderstanding was probably reinforced by the compiler's own diagnostics in Swift 3).

assumingMemoryBound is also currently the only way to call out to a C API that takes char * passing it an UnsafeRawPointer. So you may need to use it in Swift/C wrapper APIs with a comment explaining why it's safe here, but other developers shouldn't blindly copy the style.

[weissi]

@atrick Thanks, so I’m 100% on board that UnsafeRawBufferPointer is the right type as it’s a collection of UInt8 too, I just didn’t know that and therefore assumed binding the memory once and then ‘assuming’ it’s type is the best way. Clearly it’s not. I will make all the changes necessary to fully change that (this PR is just the start).

I was just worried that I fundamentally misunderstood the APIs because you said it would fail a memory model verifier. But now I see what you mean: Our code might be correct but it’s hard to know for someone without intricate ByteBuffer code knowledge as you need to read the whole code to convince yourself that we correctly bind the memory (and never rebind it). Also it’s a poor example as people might just copy the ‘assumeMemoryBound(to:)’ to random other code bases that are not as diligent with binding it to the correct type.

If that’s all accurate then be reassured that I’ll deal with all that after I’m back from my holiday.

[atrick]

I was just worried that I fundamentally misunderstood the APIs because you said it would fail a memory model verifier. But now I see what you mean: Our code might be correct but it’s hard to know for someone without intricate ByteBuffer code knowledge as you need to read the whole code to convince yourself that we correctly bind the memory (and never rebind it).

Yes. Exactly!

Also it’s a poor example as people might just copy the ‘assumeMemoryBound(to:)’ to random other code bases that are not as diligent with binding it to the correct type.

Yes. Exactly!

So, if you are always binding the buffer to UInt8 when you initialize the memory (via initializeMemory or bindMemory, then your source code would pass hypothetical memory model verification. I just wasn't sure that was the case. e.g. Maybe you pass uninitialized memory to a C routine that writes the data.

[Lukasa]

LGTM, thanks for the report @atrick!

[Lukasa]

Hrm, the failed test output is extremely garbled: I can't make out an actual error in there. I'll just kick it again.

[Lukasa]

@swift-nio-bot test this please

[Lukasa]

Hrm, looks like the CI is broken.

[tomerd]

@swift-nio-bot test this please