Define JNIEXPORT as visibility default with GCC? (original) (raw)

David DeHaven david.dehaven at oracle.com
Fri Mar 8 17:45:06 UTC 2013

Those changes look fine to me. None of those typedefs make sense with JNIEXPORT since they're only used locally, and most are scoped to a single function…

I agree with the __has_attribute comment.

The next step would be to try setting -fvisibility=hidden and -fvisibility-inlines-hidden :) I encountered a few issues trying to get that working.

-DrD-

More changes. I discovered that with JNIEXPORT defined to use the attribute, typedefs that included JNIEXPORT would generate a gcc -Wattribute warning warning: visibility attribute ignored. These new warnings are annoying, but arguably, the use of JNIEXPORT in a typedef was a bug all along - JNIEXPORT should be all about actual symbols, not types. (But I couldn't find any actual spec for JNIEXPORT; could somebody please fix that?). So I removed all occurences of JNIEXPORT inside a typedef that caused a -Wattribute warning.

http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ Martin On Tue, Mar 5, 2013 at 5:45 PM, Martin Buchholz <martinrb at google.com> wrote:

Another rev of this change. I added the condition check for gcc > 4.2 (I don't much care either way)

I just ran "clang" for the first time, and was surprised that clang has values of GNUC == 4 and GNUCMINOR == 2 _I think we should keep the hasattribute check, because it seems clearly correct and recommended by http://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros _and with luck, hasattribute will become a minor industry standard from the innovators in llvm-land. I really like the design of their extension mechanisms.

On Wed, Feb 27, 2013 at 3:07 PM, Martin Buchholz <martinrb at google.com>wrote: Here's the latest version of the proposed patch: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ that has been tested, but only with gcc 4.6, but is also written to work with llvm.

More information about the core-libs-dev mailing list