-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add Kotlin Native implementation #29
Conversation
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.
Hey @sebleclerc. I have some time this week to devote to pbandk, so I'd like to help in whatever way I can to get this PR finished and merged in. I've added a few comments below. Aside from those, I think the only remaining items on this PR are:
- Get CI passing (looks like it's currently failing because you need to re-generate the well-known types and commit them to git).
- Add implementations of
timestampToString()
andstringToTimestamp()
inpbandk/impl/Util.kt
(see Add Kotlin Native implementation #29 (comment)). I can take a stab at these if you're not sure about how to implement them. - I can't seem to get native conformance binaries to build anymore. This definitely worked for me with one of the earlier versions of your PR. But now when I run
./gradlew :conformance:native:build
, I don't get any binaries in theconformance/native/build/
directory. Are you able to build them?
val Int.zigZagEncoded get() = (this shl 1) xor (this shr 31) | ||
|
||
// Note: the right-shift must be arithmetic | ||
val Long.zigZagEncoded get() = (this shl 1) xor (this shr 63) |
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.
These should probably be marked internal
so they don't pollute the auto-complete for developers who are using pbandk in their project.
@garyp You cab have a look at timestamp / string conversion. I’ll double check on native build. If you get the the local build, you can have a look at conformance tests. And feel free to help to whatever you feel. |
Kotlin native further changes
@garyp Your PR has been merged inside my branch. Waiting on the CI to run to make sure everything is OK. |
Originally based on cretz/pb-and-k#15. Fixes This is a rebase of #29 onto the latest version of master.
Originally based on cretz/pb-and-k#15. Fixes This is a rebase of #29 onto the latest version of master.
Originally based on cretz/pb-and-k#15. This is a rebase of #29 onto the latest version of master. Fixes #19.
Merged via #76. |
No description provided.