Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262883002:
Handle @Annotated parameters when generating JNI headers from Java. (Closed)
|
Created:
4 years, 4 months ago by Bernhard Bauer Modified:
4 years, 3 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle @Annotated parameters when generating JNI headers from Java.
Committed: https://crrev.com/3f004b90c5bd61521ff170228b43fd37dd6998f3
Cr-Commit-Position: refs/heads/master@{#414419}
Patch Set 1 #Patch Set 2 : x #Patch Set 3 : x #Patch Set 4 : x #
Total comments: 4
Patch Set 5 : review #
Messages
Total messages: 36 (25 generated)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bauerb@chromium.org changed reviewers: + dgn@chromium.org, torne@chromium.org
Please review. Thanks!
peconn@chromium.org changed reviewers: + peconn@chromium.org
https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:361: if items[0][0] is '@': It'd be really cool if we could do some magic that converts the int to an appropriate enum if this is a Java annotation generated from a C++ enum.
https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:361: if items[0][0] is '@': On 2016/08/23 15:30:12, PEConn wrote: > It'd be really cool if we could do some magic that converts the int to an > appropriate enum if this is a Java annotation generated from a C++ enum. Hm, but we only use @IntDef enums, which are just integers under the hood anyway.
https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:361: if items[0][0] is '@': I'd prefer "if items[0].startswith('@')" - and also can't there be more than one annotation? (and parameters to the annotation and so on?) Making this fully general is probably a pain I guess.. And yes, it'd be nice to handle enums/etc properly; if some feels like implementing that later go ahead ;p
Ah that's great, thanks for the change! SnippetsBridge.java lgtm
On 2016/08/23 15:33:08, Bernhard Bauer wrote: > https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... > File base/android/jni_generator/jni_generator.py (right): > > https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... > base/android/jni_generator/jni_generator.py:361: if items[0][0] is '@': > On 2016/08/23 15:30:12, PEConn wrote: > > It'd be really cool if we could do some magic that converts the int to an > > appropriate enum if this is a Java annotation generated from a C++ enum. > > Hm, but we only use @IntDef enums, which are just integers under the hood > anyway. They're integers *in java*, but if it's generated from a C++ enum originally then it could, meaningfully, be typecast back to that C++ enum type when passing from Java to C++. This would require that the annotation mention what enum type it was, probably, and I dunno how complicated it would be to plumb this all - so definitely let's do this CL anyway, but if someone wanted to try "proper" enum support I'll review the change ;p
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... File base/android/jni_generator/jni_generator.py (right): https://codereview.chromium.org/2262883002/diff/60001/base/android/jni_genera... base/android/jni_generator/jni_generator.py:361: if items[0][0] is '@': On 2016/08/23 15:40:03, Torne wrote: > I'd prefer "if items[0].startswith('@')" - and also can't there be more than one > annotation? (and parameters to the annotation and so on?) Making this fully > general is probably a pain I guess.. > > And yes, it'd be nice to handle enums/etc properly; if some feels like > implementing that later go ahead ;p OK, I'm now handling any number of annotations, but no parameters yet. Yes, at some point it would probably start making more sense to use a proper Java parser.
This is at least the fifth time someone's said "we can just add one more thing but then we'll have to switch to a proper java parser", so yaknow. I'm still pretty proud of the regex magic that deletes comments accurately. :p LGTM
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2262883002/#ps80001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Handle @Annotated parameters when generating JNI headers from Java. ========== to ========== Handle @Annotated parameters when generating JNI headers from Java. Committed: https://crrev.com/3f004b90c5bd61521ff170228b43fd37dd6998f3 Cr-Commit-Position: refs/heads/master@{#414419} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3f004b90c5bd61521ff170228b43fd37dd6998f3 Cr-Commit-Position: refs/heads/master@{#414419} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
