Fix old cast style use in generated file and support lib. #453
Conversation
|
Can you please re-generate the code for the example and test cases to show the impact of the generator changes? Also please ensure the tests build and pass. You can run |
canatella
force-pushed
the
fix-old-cast-style-use
branch
from
d73142c to
06326d1
Compare
|
There you go! Test are passing now and generated code looks ok. |
There was a problem hiding this comment.
Thanks for updating. Note that Djinni is on minimal maintenance, bugfixes-only, so don't expect a fast turnaround on reviews. I'm willing to merge this in some form, though. See my inline comments for some suggestsions, though. The current implementation feels messy.
Note that if you want this merged you'll also need to sign the CLA here: https://opensource.dropbox.com/cla/individual/
| ::djinni::Optional<std::experimental::optional, ::djinni::I64>::toCpp(jniEnv, jniEnv->GetObjectField(j, data.field_mOSixtyfour)), | ||
| ::djinni::Optional<std::experimental::optional, ::djinni::F32>::toCpp(jniEnv, jniEnv->GetObjectField(j, data.field_mOFthirtytwo)), | ||
| ::djinni::Optional<std::experimental::optional, ::djinni::F64>::toCpp(jniEnv, jniEnv->GetObjectField(j, data.field_mOFsixtyfour))}; | ||
| return {::djinni::Bool::toCpp(jniEnv, (jniEnv->GetBooleanField(j, data.field_mB))), |
There was a problem hiding this comment.
This seems to be generating a lot of extra parens, as in this line. Can you make that conditional so they're only generated when necessary as part of generating a cast expression, not always?
There was a problem hiding this comment.
Due to the problem with the closing parentheses (see my following comments), it would means that toJniCall should also return a value saying if there is a need to add the closing parentheses or not which starts to get things more complicated than they should be I think. If you have an idea on how to fix all those problems, I'll gladly give it a shot.
| case p: MPrimitive => f(if (needRef) "Object" else IdentStyle.camelUpper(p.jName)) | ||
| case MString => "(jstring)" + f("Object") | ||
| case p: MPrimitive => "(" + f(if (needRef) "Object" else IdentStyle.camelUpper(p.jName)) | ||
| case MString => "reinterpret_cast<jstring>(" + f("Object") |
There was a problem hiding this comment.
Generating the open-paren and close-paren at widely-separated places in the code seems confusing, and likely to miss some cases. Why can't the close-paren be generated here so it's directly part of the cast expression?
There was a problem hiding this comment.
The problem lies in JNIGenerator.scala:288 where the expression to be casted is not entirely passed to the toJniCall method. I could add a function to generate the whole expression to be casted and then send that to toJniCall. It was not a problem before with c-style cast of course.
There was a problem hiding this comment.
So after looking at it again, it does not really make sense to try to do that. Line 288 generates the first line of the call: something like
auto jret = reinterpret_cast<jstring>(jniEnv->CallObjectMethod(
Then on line 294 it uses that string length to generate properly aligned method call so it means the output of toJniCall should depends on the length of its output... Hoping I am clear here ;)
|
Thanks for your effort! This repo is no longer supported by Dropbox and about to be archived. I'm not going to try to merge this PR at the last minute, but I'll leave it open for reference by anyone who encounters the same problem. |
Fixes LLVM warning: use of old-style cast [-Wold-style-cast]