1049105 - Remove generateStatic parameter for WrapElementForJNI annotation

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Having the generateStatic = true such as at [1] seems redundant. The function is already static and the codegen tool that uses the annotation already produces a static wrapper based on the function's static-ness [2]. If the two pieces of information conflict then the codegen tool will generate a static wrapper for a non-static function but it's not clear what will happen if anybody attempts to call it.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java?rev=4cbef502c1dc#455
[2] http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/CodeGenerator.java?rev=8fac243930da#100

Comment 1

[Chris Kitching [:ckitching]]

I think this is left over from the first revision of the code generator that didn't generate a C++ class for each wrapped class, but instead was generating wrappers in a single, large class.

This option could still be meaningfully used: should you want a static C++ method that corresponds to a non-static Java method. Since JNI wrapper methods take the Java instance as a parameter, that (I think) would still work. It'd just be a really stupid thing to do.

This option, then, is mostly a footgun, and one that doesn't provide a particularly useful feature. Let's get rid of it.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Remove generateStatic option from JNI wrapper generator

That became quite the cleanup patch, and didn't change the generated code at all, so looks like victory.

Well spotted!

Comment 3

[Chris Kitching [:ckitching]]

Attachment: V2: Remove generateStatic option from JNI wrapper generator

Comment 4

[Chris Kitching [:ckitching]]

Attachment: annotationConstraints.patch

And I just remembered I had this patch lying around in one of my queues: makes it impossible to put the annotations on element types where they make no sense. Will help IDE users avoid unfortunate compile errors (and give others a less confusing one than <evil annotation processor stacktrace>)

Might be worth having, too?

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Make annotation processor compile with Xlint:all

Also found this one lying around: makes the annotation processor have no warnings with linting turned on (which I think I wrote as part of my work to make everything compile with JDK 8, since more warnings are on by default on that compiler).

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8468098 [details] [diff] [review]
V2: Remove generateStatic option from JNI wrapper generator

Review of attachment 8468098 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/annotationProcessors/CodeGenerator.java
@@ +408,5 @@
>      }
>  
>      /**
>       * Generates the method body of the C++ wrapper function for the Java method indicated.
> +     * 

trailing ws

::: mobile/android/base/GeckoAppShell.java
@@ +451,5 @@
>              reportJavaCrash(getStackTraceString(e));
>          }
>      }
>  
> +    @WrapElementForJNI()

Remove parens, here and in the next two below as well.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8468134 [details] [diff] [review]
Make annotation processor compile with Xlint:all

Review of attachment 8468134 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly rubber-stamping this (and the previous patch). As long as it generates the same code and compiles on platforms people are using I'm fine with it.

Comment 8

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/9544fda01afc
https://hg.mozilla.org/integration/fx-team/rev/aee60fb34b32
https://hg.mozilla.org/integration/fx-team/rev/a45200559a09

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9544fda01afc
https://hg.mozilla.org/mozilla-central/rev/aee60fb34b32
https://hg.mozilla.org/mozilla-central/rev/a45200559a09