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

jni boilerplate: c -> cpp #516

Closed
wants to merge 2 commits into from
Closed

jni boilerplate: c -> cpp #516

wants to merge 2 commits into from

Conversation

pazos
Copy link
Member

@pazos pazos commented Sep 30, 2024

It isn't intended to go in for a while as it is untested.

pros:

  • It is easier to read for a ramdom lurker.
  • C/JNI boilerplate should be banned from existence.

cons:

  • It is c++

but we already use c++ for the rest of boilerplate, so why not here too? :)


This change is Reviewable

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

No real opinion. I suppose "real" objects might have some advantages over pointers but I thought it was really just a slightly different syntax for the exact same thing.

@pazos
Copy link
Member Author

pazos commented Sep 30, 2024

No real opinion. I suppose "real" objects might have some advantages over pointers but I thought it was really just a slightly different syntax for the exact same thing.

They do the same thing. The c version also showcases what the cpp does under the hood.

From a reading perspective the cpp version is way easier. Modern "source readers" are used to deal with instances of things and are self aware, so the construct:
(vm->AttachCurrentThread(&env, NULL) == 0)) is easier to grasp than
((*vm)->AttachCurrentThread(vm, &env, NULL) == 0)))

They are also easier to think and write. Hence cpp elsewhere :)

But, yeah: really just a slightly different syntax for the exact same thing. is 100% correct :)

@pazos pazos closed this Oct 2, 2024
@Frenzie
Copy link
Member

Frenzie commented Oct 2, 2024

Not convinced after all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants