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

Android: Remove unnecessary JNI function declarations #8893

Merged
merged 1 commit into from Oct 21, 2020

Conversation

JosJuice
Copy link
Member

We generally have no reason to call these functions on our own, so there's not much reason to declare them, especially not in the cpp file where they're defined. In case we ever do get a reason to do it, we can add declarations for just the functions that need them.

@lioncash
Copy link
Member

Wouldn't this cause -Wmissing-declarations warnings?

@JosJuice
Copy link
Member Author

I don't get any such warnings in the log when building this change.

@Ebola16
Copy link
Member

Ebola16 commented Jun 26, 2020

I was wondering about this too. From what I've read, forward declarations help reduce building time if a method has the same name and different parameters. NativeLibrary.run() seems to fall in this category but I'm not sure if the involvement of the JNI makes a difference here. As long as both variants of NativeLibrary.run() work as expected, build time isn't significantly slower, and the relevant object files files aren't bloated, this PR seems fine. Unfortunately I don't know what would qualifly as "significantly slower".

@JosJuice
Copy link
Member Author

I don't see why adding a forward declaration would improve the build time. A common thing to do with forward declarations is to replace an include with a forward declaration, which does save build time by skipping the include, but includes are not relevant for what we are doing here.

NativeLibrary.run is only overloaded from Java's perspective. From C++'s perspective, the two variants of it have different names. (I believe JNI was designed this way for compatibility with C, which does not support function overloading.)

Copy link
Member

@Ebola16 Ebola16 left a comment

Choose a reason for hiding this comment

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

I'll assume I read a poorly worded stack overflow comment that actually meant forward declarations improve build time when compared to using an include. If that's the case LGTM.

We generally have no reason to call these functions on our own, so
there's not much reason to declare them, especially not in the cpp
file where they're defined. In case we ever do get a reason to do
it, we can add declarations for just the functions that need them.
@leoetlino leoetlino merged commit a8b7c3b into dolphin-emu:master Oct 21, 2020
10 checks passed
@JosJuice JosJuice deleted the android-jni-declarations branch October 21, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants