Skip to content

Commit 3ac9e71

Browse files
wilhuffa-maurice
wilhuff
authored and
a-maurice
committed
Rework ToJni conversions to enable pass-through of JNI types
Previously, all types needed to be in the JniTypeMap, but this was never intended to be the end state because it would require an entry for all subtypes of Object which couldn't scale. The new implementation based on ranked choice overload selection also resolves ambiguities. For example: unsigned char could be a uint8_t (which converts to jbyte) or the underlying type for jboolean. Absent the ranking, an argument of type unsigned char would resolve to multiple overloads and would be ambiguous. PiperOrigin-RevId: 321835773
1 parent c4731f4 commit 3ac9e71

File tree

2 files changed

+143
-17
lines changed

2 files changed

+143
-17
lines changed

‎firestore/src/jni/traits.h

Lines changed: 81 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33

44
#include <jni.h>
55

6+
#include <cstddef>
7+
68
#include "app/src/include/firebase/internal/type_traits.h"
79
#include "firestore/src/jni/jni_fwd.h"
810

@@ -36,7 +38,8 @@ template <> struct IsReference<jobjectArray> : public true_type {};
3638
// MARK: Type mapping
3739

3840
// A compile-time map from C++ types to their JNI equivalents.
39-
template <typename T> struct JniTypeMap {};
41+
template <typename T> struct JniTypeMap { using type = jobject; };
42+
4043
template <> struct JniTypeMap<bool> { using type = jboolean; };
4144
template <> struct JniTypeMap<uint8_t> { using type = jbyte; };
4245
template <> struct JniTypeMap<uint16_t> { using type = jchar; };
@@ -47,12 +50,16 @@ template <> struct JniTypeMap<float> { using type = jfloat; };
4750
template <> struct JniTypeMap<double> { using type = jdouble; };
4851
template <> struct JniTypeMap<size_t> { using type = jsize; };
4952

50-
template <> struct JniTypeMap<jobject> { using type = jobject; };
51-
template <> struct JniTypeMap<jstring> { using type = jstring; };
52-
5353
template <> struct JniTypeMap<Object> { using type = jobject; };
5454
template <> struct JniTypeMap<String> { using type = jstring; };
5555

56+
template <typename T> struct JniTypeMap<Local<T>> {
57+
using type = typename JniTypeMap<T>::type;
58+
};
59+
template <typename T> struct JniTypeMap<Global<T>> {
60+
using type = typename JniTypeMap<T>::type;
61+
};
62+
5663
template <typename T>
5764
using JniType = typename JniTypeMap<decay_t<T>>::type;
5865

@@ -70,26 +77,83 @@ using EnableForReference =
7077

7178
// MARK: Type converters
7279

73-
// Converts C++ primitives to their equivalent JNI primitive types by casting.
74-
template <typename T>
75-
EnableForPrimitive<T, JniType<T>> ToJni(const T& value) {
80+
namespace internal {
81+
82+
/**
83+
* An explicit ordering for overload resolution of JNI conversions. This allows
84+
* SFINAE without needing to make all the enable_if cases mutually exclusive.
85+
*
86+
* When finding a JNI converter, we try the following, in order:
87+
* * pass through, for JNI primitive types;
88+
* * static casts, for C++ primitive types;
89+
* * pass through, for JNI reference types like jobject;
90+
* * unwrapping, for JNI reference wrapper types like `Object` or
91+
* `Local<String>`.
92+
*
93+
* `ConverterChoice` is a recursive type, defined such that `ConverterChoice<0>`
94+
* is the most derived type, `ConverterChoice<1>` and beyond are progressively
95+
* less derived. This causes the compiler to prioritize the overloads with
96+
* lower-numbered `ConverterChoice`s first, allowing compilation to succeed even
97+
* if multiple unqualified overloads would match, and would otherwise fail due
98+
* to an ambiguity.
99+
*/
100+
template <int I>
101+
struct ConverterChoice : public ConverterChoice<I + 1> {};
102+
103+
template <>
104+
struct ConverterChoice<3> {};
105+
106+
/**
107+
* Converts JNI primitive types to themselves.
108+
*/
109+
template <typename T,
110+
typename = typename enable_if<IsPrimitive<T>::value>::type>
111+
T RankedToJni(T value, ConverterChoice<0>) {
112+
return value;
113+
}
114+
115+
/**
116+
* Converts C++ primitive types to their equivalent JNI primitive types by
117+
* casting.
118+
*/
119+
template <typename T,
120+
typename = typename enable_if<IsPrimitive<JniType<T>>::value>::type>
121+
JniType<T> RankedToJni(T value, ConverterChoice<1>) {
76122
return static_cast<JniType<T>>(value);
77123
}
78124

79-
// Converts JNI wrapper reference types (like `const Object&`) and any ownership
80-
// wrappers of those types to their underlying `jobject`-derived reference.
81-
template <typename T>
82-
EnableForReference<T, JniType<T>> ToJni(const T& value) {
83-
return value.get();
125+
/**
126+
* Converts direct use of a JNI reference types to themselves.
127+
*/
128+
template <typename T,
129+
typename = typename enable_if<IsReference<T>::value>::type>
130+
T RankedToJni(T value, ConverterChoice<2>) {
131+
return value;
84132
}
85-
template <typename T, typename J = typename T::jni_type>
86-
J ToJni(const T& value) {
133+
134+
#if defined(_STLPORT_VERSION)
135+
using nullptr_t = decltype(nullptr);
136+
#else
137+
using nullptr_t = std::nullptr_t;
138+
#endif
139+
140+
inline jobject RankedToJni(nullptr_t, ConverterChoice<2>) { return nullptr; }
141+
142+
/**
143+
* Converts wrapper types to JNI references by unwrapping them.
144+
*/
145+
template <typename T, typename J = JniType<T>>
146+
J RankedToJni(const T& value, ConverterChoice<3>) {
87147
return value.get();
88148
}
89149

90-
// Preexisting JNI types can be passed directly. This makes incremental
91-
// migration possible. Ideally this could eventually be removed.
92-
inline jobject ToJni(jobject value) { return value; }
150+
} // namespace internal
151+
152+
template <typename T>
153+
auto ToJni(const T& value)
154+
-> decltype(internal::RankedToJni(value, internal::ConverterChoice<0>{})) {
155+
return internal::RankedToJni(value, internal::ConverterChoice<0>{});
156+
}
93157

94158
} // namespace jni
95159
} // namespace firestore

‎firestore/src/tests/jni/traits_test.cc

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,17 @@
44

55
#include <limits>
66

7+
#include "firestore/src/jni/env.h"
78
#include "firestore/src/jni/object.h"
9+
#include "firestore/src/jni/ownership.h"
10+
#include "firestore/src/jni/string.h"
811
#include "firestore/src/tests/firestore_integration_test.h"
912
#include "gtest/gtest.h"
1013

1114
namespace firebase {
1215
namespace firestore {
1316
namespace jni {
17+
namespace {
1418

1519
using testing::StaticAssertTypeEq;
1620

@@ -28,6 +32,11 @@ void ExpectConvertsPrimitive() {
2832
EXPECT_EQ(jni_value, static_cast<J>(cpp_value));
2933
}
3034

35+
class TestObject : public Object {
36+
public:
37+
using Object::Object;
38+
};
39+
3140
TEST_F(TraitsTest, ConvertsPrimitives) {
3241
ExpectConvertsPrimitive<bool, jboolean>();
3342
ExpectConvertsPrimitive<uint8_t, jbyte>();
@@ -40,6 +49,18 @@ TEST_F(TraitsTest, ConvertsPrimitives) {
4049
ExpectConvertsPrimitive<size_t, jsize>();
4150
}
4251

52+
TEST_F(TraitsTest, PassesThroughJniPrimitives) {
53+
ExpectConvertsPrimitive<jboolean, jboolean>();
54+
ExpectConvertsPrimitive<jbyte, jbyte>();
55+
ExpectConvertsPrimitive<jchar, jchar>();
56+
ExpectConvertsPrimitive<jshort, jshort>();
57+
ExpectConvertsPrimitive<jint, jint>();
58+
ExpectConvertsPrimitive<jlong, jlong>();
59+
ExpectConvertsPrimitive<jfloat, jfloat>();
60+
ExpectConvertsPrimitive<jdouble, jdouble>();
61+
ExpectConvertsPrimitive<jsize, jsize>();
62+
}
63+
4364
TEST_F(TraitsTest, ConvertsObjects) {
4465
Object cpp_value;
4566
jobject jni_value = ToJni(cpp_value);
@@ -53,6 +74,46 @@ TEST_F(TraitsTest, ConvertsObjects) {
5374
EXPECT_EQ(jni_value, nullptr);
5475
}
5576

77+
TEST_F(TraitsTest, ConvertsStrings) {
78+
Env env;
79+
80+
String empty_value;
81+
jstring jni_value = ToJni(empty_value);
82+
EXPECT_EQ(jni_value, nullptr);
83+
84+
Local<String> cpp_value = env.NewStringUtf("testing");
85+
jni_value = ToJni(cpp_value);
86+
EXPECT_EQ(jni_value, cpp_value.get());
87+
88+
jstring jstring_value = nullptr;
89+
jni_value = ToJni(jstring_value);
90+
EXPECT_EQ(jni_value, nullptr);
91+
}
92+
93+
TEST_F(TraitsTest, ConvertsArbitrarySubclassesOfObject) {
94+
TestObject cpp_value;
95+
jobject jni_value = ToJni(cpp_value);
96+
EXPECT_EQ(jni_value, nullptr);
97+
}
98+
99+
TEST_F(TraitsTest, ConvertsOwnershipWrappers) {
100+
StaticAssertTypeEq<JniType<Local<Object>>, jobject>();
101+
StaticAssertTypeEq<JniType<Global<String>>, jstring>();
102+
StaticAssertTypeEq<JniType<const Local<String>&>, jstring>();
103+
104+
Local<Object> local_value;
105+
jobject jni_value = ToJni(local_value);
106+
EXPECT_EQ(jni_value, nullptr);
107+
108+
Local<TestObject> test_value;
109+
jni_value = ToJni(test_value);
110+
EXPECT_EQ(jni_value, nullptr);
111+
112+
Global<Object> global_value;
113+
jni_value = ToJni(global_value);
114+
EXPECT_EQ(jni_value, nullptr);
115+
}
116+
56117
// Conversion implicitly tests type mapping. Additionally test variations of
57118
// types that should be equivalent.
58119
TEST_F(TraitsTest, DecaysBeforeMappingTypes) {
@@ -68,6 +129,7 @@ TEST_F(TraitsTest, DecaysBeforeMappingTypes) {
68129
StaticAssertTypeEq<JniType<Object&&>, jobject>();
69130
}
70131

132+
} // namespace
71133
} // namespace jni
72134
} // namespace firestore
73135
} // namespace firebase

0 commit comments

Comments
 (0)