search

timob / jnigi

Golang Java JNI library
BSD 2-Clause "Simplified" License
166 stars 44 forks source link

Export FindClass in Env #62

Closed londek closed 2 years ago

londek commented 2 years ago

Closes #61

timob commented 2 years ago

Looks good, would be good to add the documentation comment.

londek commented 2 years ago

Hey, can you please check now?

timob commented 2 years ago

I think there is a problem with the last commit, on line 102.

londek commented 2 years ago

Sorry for hussle, such a small typo and would break entire code. I added support for int32 in helper.go. I don't think it deserves its own pull request, it's not complex change at all. Hopefully now its alright.

timob commented 2 years ago

Why do we need to handle int32 in that function since it's internal and we never pass an int32 value to it as first argument in jnigi.go? Is there a reason you want these changes?

Java doesn't have an equivalent type to int32, so we will never receive a value we need to assign.

I can understand the handling of int32 destination in int case clause, but this should be in a different PR/Issue. (Maybe we shouldn't convert go int at all, just int32?)

timob commented 2 years ago

I've added https://github.com/timob/jnigi/issues/64. If that looks good, i will do a PR for this. For this PR if you remove the change to helper.go I'll merge.

londek commented 2 years ago

Hey, I didn't have time to reply, it was really busy week for me. I guess you are right about creating new PR, I'm gonna remove latest commit in few minutes Thanks ;)

timob commented 2 years ago

Great 🎉, thanks!