Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Int(buffer:) initializer to FixedWidthInteger types #3047

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

natikgadzhi
Copy link
Contributor

@natikgadzhi natikgadzhi commented Jan 12, 2025

Motivation:

Makes it possible to make integers out of ByteBuffers directly with an initializer. I.e. UInt32(buffer: ByteBuffer(bytes: [0, 1, 2, 3])). Closes #3012.

Modifications:

  • Adds the Int(buffer:) initializer in ByteBuffer-int.swift
  • Adds tests in ByteBufferTest.swift. Holy hell this file is huge.
  • Note that Int(buffer:) will crash if the buffer does not have enough bytes in it to represent the desired integer type.
  • It also does not expose endianness and uses Endianness.host, to keep the public API clean and simple. It's a convenience initializer, so I thought keeping it minimal is a good idea.

Result:

Nicer direct initializer for integers from ByteBuffers.

Motivation:

Makes it possible to make integers out of ByteBuffers directly with an initializer. I.e. `UInt32(buffer: ByteBuffer(bytes: [0, 1, 2, 3]))`.

Modifications:

- Adds the `Int(buffer:)` initializer in `ByteBuffer-int.swift`
- Adds tests in `ByteBufferTest.swift`. _Holy hell this file is huge._

Result:

Nicer direct initializer for integers from ByteBuffers.
@natikgadzhi
Copy link
Contributor Author

Hmmmm. Unlike Array(buffer:), this will fail if there's not enough stuff in the Buffer. The other two just read whatever is readable, so they should not fail arbitrarily if the user didn't check the length.

Perhaps it's worth adding a check and making this throwing, or keeping this optional? Or documenting that the user should ensure the length of the buffer is enough?

@natikgadzhi
Copy link
Contributor Author

I checked the docs for ByteBuffer, and they already explain that ByteBuffer can cast to/from "various unsigned int types", so I think that's enough.

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jan 13, 2025
///
/// - Returns: The integer value read from the buffer
@inlinable
public init(buffer: ByteBuffer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely be failable, so that we can avoid the force-unwrap. We also want to add tests.

There's a separate question about whether this should be exactly the right size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weissi strong opinions on checking that the buffer size matches the int size exactly?

If we should verify the size match, the most straightforward way would be to check buffer.readableBytes == MemoryLayout<Self.self>.size, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm definitely in favour of checking that this is exactly the right size. It's weird to left garbage around, if that's expected readInteger is the right tool. Endianness should also be configurable (but defaulted to .host) Something like

var buffer = buffer
guard let value = buffer.readInteger(endianness: endianness, as: Self.self), buffer.readableBytes == 0 else {
    return nil
}
return value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FixedWidthInteger.init(buffer: ByteBuffer) missing
3 participants