-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
JNI bindings for Roc #2
Conversation
Wow, that's impressive! I'll review it in the coming days. |
Huge work! I'm still reviewing the code, but here are some preliminary comments / questions (mostly cosmetic):
Note: if you agree with some points, but would not like to implement them in this PR, we can create individual issues for them. |
I've tried to run
Is it a bug? |
Thank you for your feedback.
I'll work soon implementing all those points except points 6 and 9 that can be implemented on separate issue. |
Yes |
Well, actually I think we can just allocate a private array of ROC_ADDRESS_SIZE bytes in Java code and pass it to C to use as raw memory for BUT. In 0.2, So, while I think we can resolve this without some kind of finalizers for |
And some final questions / suggestions:
|
A typo? Because it's |
In general, the code looks great and I did not find any serious issues, and most of my comments are actually cosmetic. Big thanks, and a special thank for tests and documentation! I hope we didn't miss any memory leaks in the C code. Maybe later we can add support for some leak detection tools like clang sanitizers and add them to CI. BTW, did you create all those enums by hand or used some kind of automation? |
Could you please also leave a comment in roc-streaming/roc-toolkit#233 so that I could re-assign it to you? |
1.-2. Ok |
Yes definitely I meant |
Thank you, it's a pleasure to contribute to this awesome project. Yes surely we can 👍 Unfortunately yes I've created enums by hand because I've no experience with any jni generator tools. |
…, logger null handler & mutex fix
Let's keep it as is until libroc will have a more granular error reporting. Then we'll be able to translate roc errors to exception more accurately. |
|
Cool, except one minor issue (see above) and the failing tests, the PR looks good. |
Ok 👍 |
The build on my machine is actually passing. I've to investigate that problem deeply |
I've created a few issues for future improvements: https://github.com/roc-project/roc-java/issues |
FYI:
|
Replacing enum initializers with constants fixes the issue:
So the problem is with getRocLogNone() etc.
|
Adding explicit call to RocLibrary.loadLibrary to the beginning of each LoggerTest test fixes the issue as well:
Maybe the order of:
and:
is not guaranteed? |
One stupid way to fix it:
Maybe you'll find a better approach. |
Yes enum initializer is executed before static block initializer. I'll fix soon in next commit |
Tests are passing now. LGTM, thank you! |
@MatteoArella Do you plan to continue working on this in future? Please let me know if you would like to take maintainership and I'll add you to github team. |
Yes sure I'll be glad to work for this project |
Great, sent you an invite! |
You'll be able to manage issues & pull requests on this repo, and push to non-master branches. (The master branch is protected, so that we should use PRs to deliver changes to it). Let me know if you have any trouble. |
This PR includes: