-
Notifications
You must be signed in to change notification settings - Fork 235
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
feat(kotlin): Add Unsigned Primitive Support #1886
Conversation
kotlin/src/main/kotlin/org/apache/fury/serializer/kotlin/UnsignedSerializer.kt
Outdated
Show resolved
Hide resolved
kotlin/src/main/kotlin/org/apache/fury/serializer/kotlin/UnsignedSerializer.kt
Outdated
Show resolved
Hide resolved
kotlin/src/main/kotlin/org/apache/fury/serializer/kotlin/UnsignedSerializer.kt
Show resolved
Hide resolved
kotlin/src/main/kotlin/org/apache/fury/serializer/kotlin/UnsignedSerializer.kt
Show resolved
Hide resolved
Co-authored-by: Shawn Yang <[email protected]>
Co-authored-by: Shawn Yang <[email protected]>
Co-authored-by: Shawn Yang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
This reverts commit 8538e3e.
Just to let you know read/writeVarUint16 method doesn't exist in |
My bad, MemoryBuffer didn't provide read/writeVarUint16 method, we can keep using read/writeVarUint32, the performance will be same |
Will doing a reinterpret_cast to short be more compact (kotlin toXYZ methods are |
kotlin/README.md
Outdated
@@ -9,7 +9,7 @@ Fury Kotlin provides additional tests and implementation support for Kotlin type | |||
Fury Kotlin is tested and works with the following types: | |||
|
|||
- primitives: `Byte`, `Boolean`, `Int`, `Short`, `Long`, `Char`, `Float`, `Double`, `UByte`, `UShort`, `UInt`, `UShort`, `UInt`. | |||
- `Byte`, `Boolean`, `Int`, `Short`, `Long`, `Char`, `Float`, `Double` works out of the box with the default java implementation. | |||
- `Byte`, `Boolean`, `Int`, `Short`, `Long`, `Char`, `Float`, `Double` works out of the box with the default java implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we replace all default java implementation
with default fury java implementation
go back to read/writeVarInt because from code it looks like it'll do the right thing (maybe extra branching) This reverts commit ee9a5fa.
looks like read/writeUint32 will pick the right representation. |
What does this PR do?
This PR adds unsigned primitive support to Kotlin Fury.
It also adds tests for the standard kotlin primitives(supported by fury Java), nullable primitive tests, boundary tests for unsigned serializers.
Related issues
#683
Does this PR introduce any user-facing change?
Yes it adds Unsigned support for Kotlin. There's documentation new issue (should add something to document Kotlin!)
Benchmark
N/A