apple / swift-nio

Event-driven network application framework for high performance protocol servers & clients, non-blocking.
https://swiftpackageindex.com/apple/swift-nio/documentation
Apache License 2.0
7.85k stars 633 forks source link

We should offer the same `[get/set]MultipleIntegers` as we offer for `[read/write]MultipleIntegers` methods on ByteBuffer #2736

Open FranzBusch opened 4 weeks ago

FranzBusch commented 4 weeks ago

Normally we provide all four variants of methods like get/set/read/write on the various ByteBuffer operations but we seem to missing the get/set variants for the multipleInterger based methods.

Lukasa commented 4 weeks ago

I think @weissi might make the case that this is a feature not a bug, I know he's been unhappy with the get/set methods for a while. I'm curious what he thinks.

weissi commented 4 weeks ago

I think @weissi might make the case that this is a feature not a bug, I know he's been unhappy with the get/set methods for a while. I'm curious what he thinks.

I strongly dislike get/set's prominence, yes. 99.9% of their uses are either verbose or incorrect, usually both. Particularly

// correct: Something(buffer: buffer)
//
// incorrect and often seen in the wild
buffer.getSomething(at: 0, length: buffer.readableBytes)

// correct but verbose, seldomly seen
buffer.getSomething(at: buffer.readerIndex, length: buffer.readableBytes)

is both incorrect & verbose.


Now, there are some niche use cases for our get*/sets but most people just use them becauseread/write* are mutating. So I think instead of adding get*/set*s eagerly we should offer peek* which doesn't take an at: Int parameter. So peek* ~= read* except that it doesn't read it off.


My thinking is, let's invest time in

FranzBusch commented 3 weeks ago

My use-case is the classic peek case where I need to look into an incoming packet to decide the kind of it and the send into the right child channel.

weissi commented 3 weeks ago

My use-case is the classic peek case where I need to look into an incoming packet to decide the kind of it and the send into the right child channel.

Makes sense, I think we should add peek*s for everything. That's btw also a good starter task because it really just is

@inlinable
public func peekX(length: Int) -> X? {
    return self.getX(at: self.readerIndex, length: length)
}

or

@inlinable
public func peekX(length: Int) -> X? {
    var `self` = self
    return self.readX(length: length)
}
FranzBusch commented 3 weeks ago

Yeah I am totally fine rebranding the current get methods to peek and avoid doing public get/set all together.

glbrntt commented 3 weeks ago

I really like the idea of peek, I think it covers most of the existing uses of get.

I worry slightly that get is still the obvious name to reach for and many users won't actually read the docs which should discouraging them from using it. Ideally I think get/set would still exist but on a view type so they're less prominent in the API.

weissi commented 3 weeks ago

I worry slightly that get is still the obvious name to reach for and many users won't actually read the docs which should discouraging them from using it. Ideally I think get/set would still exist but on a view type so they're less prominent in the API.

Agreed. In theory we could deprecate them but that feels like a massive overreach and abuse of the deprecation mechanism. Also it would penalise even users that use get* correctly :|.

In all honesty I don't think there's much we can (reasonably) do in the 2.x major release that isn't just improving the docs.