Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Rework expression conversion #11490

Merged
merged 3 commits into from
Apr 10, 2018
Merged

Rework expression conversion #11490

merged 3 commits into from
Apr 10, 2018

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Mar 21, 2018 •
edited
Loading

Closes #11420 & #11419.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Mar 21, 2018
@tobrun tobrun added this to the android-v6.0.0 milestone Mar 21, 2018
@tobrun tobrun self-assigned this Mar 21, 2018
@tobrun tobrun force-pushed the tvn-rework-expression-conversion branch 2 times, most recently from 73179ac to d809afa Compare March 23, 2018 17:53
@tobrun
Copy link
Member Author

tobrun commented Mar 23, 2018

@ivovandongen everything is working as before, this PR is ready for a review

@tobrun tobrun force-pushed the tvn-rework-expression-conversion branch from d809afa to 196a2a3 Compare March 24, 2018 21:21
ivovandongen
ivovandongen suggested changes Apr 5, 2018
View reviewed changes
Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting to look good!

platform/android/src/gson/json_array.cpp

for (const auto &v : values) {
auto jsonElement = JsonElement::New(env, v);
jsonArray.Call(env, addMethod, jsonElement);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a loop like this, it's important to clean up the temporary local references (jni:DeleteLocalRef(jsonElement))

platform/android/src/gson/json_array.cpp Outdated
@@ -6,6 +6,20 @@ namespace mbgl {
namespace android {
namespace gson {

jni::Object<JsonArray> JsonArray::New(jni::JNIEnv& env, std::vector<mapbox::geometry::value> values){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could pass the vector by const ref here.

platform/android/src/gson/json_element.cpp

jni::JNIEnv& env;

jni::Object<JsonElement> operator()(const JsonPrimitive::value value) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ref?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not seeing where I can add an additional const here

platform/android/src/gson/json_object.cpp

for (auto &item : values) {
jni::Object<JsonElement> jsonElement = JsonElement::New(env, item.second);
jni::String key = jni::Make<jni::String>(env, item.first);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete local references (jsonElement and key)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ref

platform/android/src/gson/json_primitive.cpp Outdated
static auto constructor = JsonPrimitive::javaClass.GetConstructor<jni::Object<java::lang::Number>>(env);
auto boxedValue = java::lang::Double::valueOf(env, value);
auto number = jni::Cast(env, boxedValue, java::lang::Number::javaClass);
return JsonPrimitive::javaClass.New(env, constructor, number);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete local references

platform/android/src/gson/json_primitive.cpp
*/
jni::Object<JsonPrimitive> operator()(const int64_t value) const {
static auto constructor = JsonPrimitive::javaClass.GetConstructor<jni::Object<java::lang::Number>>(env);
auto boxedValue = java::lang::Long::valueOf(env, value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete local references

platform/android/src/gson/json_primitive.cpp Outdated
jni::Object<JsonPrimitive> operator()(const uint64_t value) const {
// long conversion
static auto constructor = JsonPrimitive::javaClass.GetConstructor<jni::Object<java::lang::Number>>(env);
auto boxedValue = java::lang::Long::valueOf(env, value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete local references

platform/android/src/gson/json_primitive.cpp
*/
jni::Object<JsonPrimitive> operator()(const bool value) const {
static auto constructor = JsonPrimitive::javaClass.GetConstructor<jni::Object<java::lang::Boolean>>(env);
auto boxedValue = java::lang::Boolean::valueOf(env, value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete local reference

platform/android/src/style/conversion/function.hpp Outdated
jni::jobject* converted = apply_visitor(jsonEvaluator, expressionValue);

return converted;
return gson::JsonElement::New(env, expressionValue);;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional ;

platform/android/src/style/conversion/function.hpp Outdated
@@ -21,10 +22,7 @@ struct Converter<jni::jobject*, mbgl::style::CameraFunction<T>> {
Result<jni::jobject*> operator()(jni::JNIEnv& env, const mbgl::style::CameraFunction<T>& value) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, you could one step further here and type the result with high-level types as well (jni::Object<...>), like https://github.com/mapbox/mapbox-gl-native/blob/release-boba/platform/android/src/geojson/conversion/geometry.hpp#L15

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! will include this

@tobrun
Copy link
Member Author

tobrun commented Apr 5, 2018

@ivovandongen I addressed your comments in a separate commit for ease of re-review.
Thanks for 👀'in, ready for re-review.

@tobrun tobrun force-pushed the tvn-rework-expression-conversion branch from 4d58709 to 222a2c0 Compare April 5, 2018 20:36
@tobrun tobrun force-pushed the tvn-rework-expression-conversion branch from 81d3068 to 9a1c3f4 Compare April 9, 2018 18:39
@tobrun
Copy link
Member Author

tobrun commented Apr 10, 2018 •
edited
Loading

9a1c3f4 reworks passing by const reference instead of const value.

@tobrun tobrun merged commit 44ce5ab into release-boba Apr 10, 2018
@tobrun tobrun deleted the tvn-rework-expression-conversion branch April 10, 2018 09:09
This was referenced Apr 11, 2018
Closed
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ivovandongen ivovandongen ivovandongen approved these changes

Assignees

@tobrun tobrun

Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Milestone
android-v6.0.0-boba
Development

Successfully merging this pull request may close these issues.
3 participants
@tobrun @ivovandongen @lilykaiser