-
Notifications
You must be signed in to change notification settings - Fork 489
Fix old cast style use in generated file and support lib. #453
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,20 +35,20 @@ auto NativeAssortedPrimitives::toCpp(JNIEnv* jniEnv, JniType j) -> CppType { | |
::djinni::JniLocalScope jscope(jniEnv, 15); | ||
assert(j != nullptr); | ||
const auto& data = ::djinni::JniClass<NativeAssortedPrimitives>::get(); | ||
return {::djinni::Bool::toCpp(jniEnv, jniEnv->GetBooleanField(j, data.field_mB)), | ||
::djinni::I8::toCpp(jniEnv, jniEnv->GetByteField(j, data.field_mEight)), | ||
::djinni::I16::toCpp(jniEnv, jniEnv->GetShortField(j, data.field_mSixteen)), | ||
::djinni::I32::toCpp(jniEnv, jniEnv->GetIntField(j, data.field_mThirtytwo)), | ||
::djinni::I64::toCpp(jniEnv, jniEnv->GetLongField(j, data.field_mSixtyfour)), | ||
::djinni::F32::toCpp(jniEnv, jniEnv->GetFloatField(j, data.field_mFthirtytwo)), | ||
::djinni::F64::toCpp(jniEnv, jniEnv->GetDoubleField(j, data.field_mFsixtyfour)), | ||
::djinni::Optional<std::experimental::optional, ::djinni::Bool>::toCpp(jniEnv, jniEnv->GetObjectField(j, data.field_mOB)), | ||
::djinni::Optional<std::experimental::optional, ::djinni::I8>::toCpp(jniEnv, jniEnv->GetObjectField(j, data.field_mOEight)), | ||
::djinni::Optional<std::experimental::optional, ::djinni::I16>::toCpp(jniEnv, jniEnv->GetObjectField(j, data.field_mOSixteen)), | ||
::djinni::Optional<std::experimental::optional, ::djinni::I32>::toCpp(jniEnv, jniEnv->GetObjectField(j, data.field_mOThirtytwo)), | ||
::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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
::djinni::I8::toCpp(jniEnv, (jniEnv->GetByteField(j, data.field_mEight))), | ||
::djinni::I16::toCpp(jniEnv, (jniEnv->GetShortField(j, data.field_mSixteen))), | ||
::djinni::I32::toCpp(jniEnv, (jniEnv->GetIntField(j, data.field_mThirtytwo))), | ||
::djinni::I64::toCpp(jniEnv, (jniEnv->GetLongField(j, data.field_mSixtyfour))), | ||
::djinni::F32::toCpp(jniEnv, (jniEnv->GetFloatField(j, data.field_mFthirtytwo))), | ||
::djinni::F64::toCpp(jniEnv, (jniEnv->GetDoubleField(j, data.field_mFsixtyfour))), | ||
::djinni::Optional<std::experimental::optional, ::djinni::Bool>::toCpp(jniEnv, (jniEnv->GetObjectField(j, data.field_mOB))), | ||
::djinni::Optional<std::experimental::optional, ::djinni::I8>::toCpp(jniEnv, (jniEnv->GetObjectField(j, data.field_mOEight))), | ||
::djinni::Optional<std::experimental::optional, ::djinni::I16>::toCpp(jniEnv, (jniEnv->GetObjectField(j, data.field_mOSixteen))), | ||
::djinni::Optional<std::experimental::optional, ::djinni::I32>::toCpp(jniEnv, (jniEnv->GetObjectField(j, data.field_mOThirtytwo))), | ||
::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)))}; | ||
} | ||
|
||
} // namespace djinni_generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)