Skip to content

Commit

Permalink
Pack discriminator only operates on types and const auto&.
Browse files Browse the repository at this point in the history
This had inconsistent behaviour across compilers, and Ubuntu + C++20 specifically breaks.

A similar issue was encountered a while back when trying to support Windows, which was what motivated the "diminished" forms.

PiperOrigin-RevId: 712294962
  • Loading branch information
jwhpryor authored and copybara-github committed Jan 5, 2025
1 parent d855896 commit 8348bd8
Show file tree
Hide file tree
Showing 109 changed files with 6,322 additions and 1,582 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- name: test
run: bazel query --output=label 'kind("...", //...) except attr("tags", "cpp20", "//...")' | xargs bazel test --cxxopt='-Werror' --cxxopt='-std=c++17' --repo_env=CC=clang --test_output=errors
run: bazel test --cxxopt='-Werror' --cxxopt='-std=c++17' --repo_env=CC=clang --test_output=errors //implementation/legacy/...

ubuntu-latest-cpp20:
runs-on: ubuntu-latest
Expand All @@ -29,7 +29,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- name: test
run: bazel query --output=label 'kind("...", //...) except attr(tags, "cpp20", //...)' | xargs bazel test --cxxopt='-Werror' --cxxopt='-std=c++17' --repo_env=CC=clang --test_output=errors
run: bazel test --cxxopt='-Werror' --cxxopt='-std=c++17' --repo_env=CC=clang --test_output=errors //implementation/legacy/...

macos-latest-cpp20:
runs-on: macos-latest
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
bazel*
.vscode
1 change: 1 addition & 0 deletions BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ cc_library(
"//implementation/jni_helper:static_field_value",
"//metaprogramming:corpus",
"//metaprogramming:corpus_tag",
"//metaprogramming:string_literal",
],
)

Expand Down
12 changes: 10 additions & 2 deletions implementation/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,17 @@ cc_library(
hdrs = ["class_loader_ref.h"],
deps = [
":class_loader",
":class_ref",
":default_class_loader",
":global_object",
":id",
":id_type",
":jni_type",
":jvm",
":local_object",
":no_idx",
":promotion_mechanics",
":promotion_mechanics_tags",
"//:jni_dep",
"//class_defs:java_lang_classes",
"//implementation/jni_helper:jni_env",
Expand Down Expand Up @@ -923,8 +926,11 @@ cc_library(
"//:jni_dep",
"//implementation/jni_helper:lifecycle",
"//metaprogramming:invocable_map",
"//metaprogramming:invocable_map_20",
"//metaprogramming:queryable_map",
"//metaprogramming:queryable_map_20",
"//metaprogramming:string_contains",
"//metaprogramming:string_literal",
],
)

Expand Down Expand Up @@ -1071,7 +1077,7 @@ cc_test(
deps = [
"//:jni_bind",
"//:jni_test",
"//metaprogramming:concatenate",
"//metaprogramming:type_to_type_map",
"@googletest//:gtest_main",
],
)
Expand Down Expand Up @@ -1237,9 +1243,11 @@ cc_library(
":method_selection",
":no_idx",
"//:jni_dep",
"//implementation/jni_helper:invoke_static",
"//metaprogramming:invocable_map",
"//metaprogramming:invocable_map_20",
"//metaprogramming:queryable_map",
"//metaprogramming:queryable_map_20",
"//metaprogramming:string_literal",
],
)

Expand Down
3 changes: 0 additions & 3 deletions implementation/array_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@

#include <type_traits>

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "jni_bind.h"
#include "jni_test.h"

namespace {

Expand Down
4 changes: 2 additions & 2 deletions implementation/array_type_conversion_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
* limitations under the License.
*/

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <type_traits>

#include "jni_bind.h"

namespace {
Expand Down
48 changes: 20 additions & 28 deletions implementation/array_view_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/
#include <algorithm>
#include <array>

#include <gmock/gmock.h>
#include <gtest/gtest.h>
Expand Down Expand Up @@ -50,51 +51,43 @@ TEST_F(JniTest, ArrayView_CallsLengthProperly) {
TEST_F(JniTest, ArrayView_GetsAndReleaseArrayBuffer) {
EXPECT_CALL(*env_, GetBooleanArrayElements(Eq(Fake<jbooleanArray>()), _))
.WillOnce(::testing::Return(Fake<jboolean*>()));
EXPECT_CALL(*env_, ReleaseBooleanArrayElements(
Eq(Fake<jbooleanArray>()),
Eq(Fake<jboolean*>()), 0));
EXPECT_CALL(*env_, ReleaseBooleanArrayElements(Eq(Fake<jbooleanArray>()),
Eq(Fake<jboolean*>()), 0));

EXPECT_CALL(*env_, GetByteArrayElements(Eq(Fake<jbyteArray>()), _))
.WillOnce(::testing::Return(Fake<jbyte*>()));
EXPECT_CALL(*env_,
ReleaseByteArrayElements(Eq(Fake<jbyteArray>()),
Eq(Fake<jbyte*>()), 0));
EXPECT_CALL(*env_, ReleaseByteArrayElements(Eq(Fake<jbyteArray>()),
Eq(Fake<jbyte*>()), 0));

EXPECT_CALL(*env_, GetCharArrayElements(Eq(Fake<jcharArray>()), _))
.WillOnce(::testing::Return(Fake<jchar*>()));
EXPECT_CALL(*env_,
ReleaseCharArrayElements(Eq(Fake<jcharArray>()),
Eq(Fake<jchar*>()), 0));
EXPECT_CALL(*env_, ReleaseCharArrayElements(Eq(Fake<jcharArray>()),
Eq(Fake<jchar*>()), 0));

EXPECT_CALL(*env_, GetShortArrayElements(Eq(Fake<jshortArray>()), _))
.WillOnce(::testing::Return(Fake<jshort*>()));
EXPECT_CALL(
*env_, ReleaseShortArrayElements(Eq(Fake<jshortArray>()),
Eq(Fake<jshort*>()), 0));
EXPECT_CALL(*env_, ReleaseShortArrayElements(Eq(Fake<jshortArray>()),
Eq(Fake<jshort*>()), 0));

EXPECT_CALL(*env_, GetIntArrayElements(Eq(Fake<jintArray>()), _))
.WillOnce(::testing::Return(Fake<jint*>()));
EXPECT_CALL(*env_,
ReleaseIntArrayElements(Eq(Fake<jintArray>()),
Eq(Fake<jint*>()), 0));
EXPECT_CALL(*env_, ReleaseIntArrayElements(Eq(Fake<jintArray>()),
Eq(Fake<jint*>()), 0));

EXPECT_CALL(*env_, GetLongArrayElements(Eq(Fake<jlongArray>()), _))
.WillOnce(::testing::Return(Fake<jlong*>()));
EXPECT_CALL(*env_,
ReleaseLongArrayElements(Eq(Fake<jlongArray>()),
Eq(Fake<jlong*>()), 0));
EXPECT_CALL(*env_, ReleaseLongArrayElements(Eq(Fake<jlongArray>()),
Eq(Fake<jlong*>()), 0));

EXPECT_CALL(*env_, GetFloatArrayElements(Eq(Fake<jfloatArray>()), _))
.WillOnce(::testing::Return(Fake<jfloat*>()));
EXPECT_CALL(
*env_, ReleaseFloatArrayElements(Eq(Fake<jfloatArray>()),
Eq(Fake<jfloat*>()), 0));
EXPECT_CALL(*env_, ReleaseFloatArrayElements(Eq(Fake<jfloatArray>()),
Eq(Fake<jfloat*>()), 0));

EXPECT_CALL(*env_, GetDoubleArrayElements(Eq(Fake<jdoubleArray>()), _))
.WillOnce(::testing::Return(Fake<jdouble*>()));
EXPECT_CALL(*env_, ReleaseDoubleArrayElements(
Eq(Fake<jdoubleArray>()),
Eq(Fake<jdouble*>()), 0));
EXPECT_CALL(*env_, ReleaseDoubleArrayElements(Eq(Fake<jdoubleArray>()),
Eq(Fake<jdouble*>()), 0));

LocalArray<jboolean> boolean_array{AdoptLocal{}, Fake<jbooleanArray>()};
LocalArray<jbyte> byte_array{AdoptLocal{}, Fake<jbyteArray>()};
Expand All @@ -118,9 +111,8 @@ TEST_F(JniTest, ArrayView_GetsAndReleaseArrayBuffer) {
TEST_F(JniTest, LocalArrayView_AllowsCTAD) {
EXPECT_CALL(*env_, GetBooleanArrayElements(Eq(Fake<jbooleanArray>()), _))
.WillOnce(::testing::Return(Fake<jboolean*>()));
EXPECT_CALL(*env_, ReleaseBooleanArrayElements(
Eq(Fake<jbooleanArray>()),
Eq(Fake<jboolean*>()), 0));
EXPECT_CALL(*env_, ReleaseBooleanArrayElements(Eq(Fake<jbooleanArray>()),
Eq(Fake<jboolean*>()), 0));

LocalArray<jboolean> boolean_array{AdoptLocal{}, Fake<jbooleanArray>()};
ArrayView ctad_array_view{boolean_array.Pin()};
Expand Down Expand Up @@ -326,7 +318,7 @@ TEST_F(JniTest, ArrayView_RichObjectsAreIterable) {
int fake_result = 123;
for (LocalObject<kClass> obj : obj_view) {
EXPECT_CALL(*env_, CallIntMethodV).WillOnce(::testing::Return(fake_result));
EXPECT_EQ(obj("Foo"), fake_result);
EXPECT_EQ(obj.Call<"Foo">(), fake_result);
fake_result++;
}
}
Expand Down
13 changes: 12 additions & 1 deletion implementation/class.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ namespace jni {

template <typename Extends_, typename Constructors_, typename Static_,
typename Methods_, typename Fields_>
struct Class {};
struct Class {
constexpr Class() = default;
constexpr Class(const char* name) {}
};

template <typename Extends_, typename... Constructors_,
typename... StaticMethods_, typename... StaticFields_,
Expand Down Expand Up @@ -70,6 +73,14 @@ struct Class<Extends_, std::tuple<Constructors_...>,
// provided where they are and aren't present.
////////////////////////////////////////////////////////////////////////////////

// To stifle a test failure.
constexpr Class()
: Object("__JNI_BIND__NO_CLASS__"),
constructors_(Constructor<>{}),
static_(),
methods_(),
fields_() {}

// Methods + Fields.
explicit constexpr Class(const char* class_name, Methods_... methods,
Fields_... fields)
Expand Down
16 changes: 14 additions & 2 deletions implementation/class_loader_ref.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#include "class_defs/java_lang_classes.h"
#include "implementation/class_loader.h"
#include "implementation/class_ref.h"
#include "implementation/default_class_loader.h"
#include "implementation/global_object.h"
#include "implementation/id.h"
Expand All @@ -29,9 +30,11 @@
#include "implementation/jni_helper/lifecycle.h"
#include "implementation/jni_helper/lifecycle_object.h"
#include "implementation/jni_type.h"
#include "implementation/jvm.h"
#include "implementation/local_object.h"
#include "implementation/no_idx.h"
#include "implementation/promotion_mechanics.h"
#include "implementation/promotion_mechanics_tags.h"
#include "jni_dep.h"

namespace jni {
Expand Down Expand Up @@ -73,10 +76,19 @@ class ClassLoaderRef : public ClassLoaderImpl<lifecycleType> {
kDefaultClassLoader) {
ClassRef_t<JniT<jobject, class_v, class_loader_v_, jvm_v_,
0>>::PrimeJClassFromClassLoader([&]() {
// Prevent the object (which is a runtime instance of a class) from
// falling out of scope so it is not released.
// Prevent the object (which is a runtime instance of a class) from
// falling out of scope so it is not released.

#if __cplusplus >= 202002L
LocalObject loaded_class =
(*this).template Call<"loadClass">(IdClassT::kNameUsingDots);
#elif __clang__
LocalObject loaded_class =
(*this)("loadClass", IdClassT::kNameUsingDots);
#else
static_assert(
false, "JNI Bind requires C++20 (or later) or C++17 with clang.");
#endif

// We only want to create global references if we are actually going
// to use them so that they do not leak.
Expand Down
2 changes: 1 addition & 1 deletion implementation/class_loader_ref_second_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ TEST_F(JniTestWithNoDefaultJvmRef,
auto second_custom_loader_object =
class_loader.BuildLocalObject<kClass>(jint{2});

EXPECT_EQ(custom_loader_object("methodNoCrossTalk", jint{2}), 123);
EXPECT_EQ(custom_loader_object.Call<"methodNoCrossTalk">(jint{2}), 123);

TearDown();
}
Expand Down
17 changes: 8 additions & 9 deletions implementation/class_loader_ref_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include <optional>
#include <utility>

#include <gmock/gmock.h>
Expand Down Expand Up @@ -119,7 +118,7 @@ TEST_F(JniTest, LocalObject_SupportsPassingAnObjectWithAClassLoader) {
// LocalObject<kClass2, kClassLoader> a{}; // doesn't compile (good).
LocalObject<kClass1, kClassLoader> a{Fake<jobject>()};
LocalObject<kClass2> b{};
b("Foo", a);
b.Call<"Foo">(a);

default_globals_made_that_should_be_released_.clear();
}
Expand All @@ -131,7 +130,7 @@ TEST_F(JniTestForClassLoaders,
LocalClassLoader<kClassLoader> local_class_loader{Fake<jobject>()};
auto a = local_class_loader.BuildLocalObject<kClass1>();
LocalObject<kClass2> b{a};
b("Foo", a);
b.Call<"Foo">(a);

default_globals_made_that_should_be_released_.clear();
TearDown();
Expand All @@ -144,7 +143,7 @@ TEST_F(JniTestForClassLoaders,
LocalClassLoader<kClassLoader> local_class_loader{Fake<jobject>()};
auto a = local_class_loader.BuildGlobalObject<kClass1>();
LocalObject<kClass2> b{a};
b("Foo", a);
b.Call<"Foo">(a);

default_globals_made_that_should_be_released_.clear();
TearDown();
Expand All @@ -161,7 +160,7 @@ TEST_F(JniTestForClassLoaders,
LocalObject<kClass1, kClassLoader> a =
local_class_loader.BuildLocalObject<kClass1>();
LocalObject<kClass2> b{a};
b("Foo", a);
b.Call<"Foo">(a);

TearDown();
}
Expand All @@ -173,7 +172,7 @@ TEST_F(JniTestForClassLoaders, ClassLoaderRefTest_ConstructsFromRValue) {
LocalObject<kClass1, kClassLoader, kJvm> b{
local_class_loader.BuildLocalObject<kClass1>()};

LocalObject<kClass2, kClassLoader, kJvm> c{b("Foo")};
LocalObject<kClass2, kClassLoader, kJvm> c{b.Call<"Foo">()};

TearDown();
}
Expand All @@ -187,7 +186,7 @@ TEST_F(JniTestForClassLoaders,
LocalObject<kClass1> a{};
LocalObject<kClass2, kClassLoader, kJvm> b =
local_class_loader.BuildLocalObject<kClass2>(a);
b("Foo", a);
b.Call<"Foo">(a);

TearDown();
}
Expand Down Expand Up @@ -236,7 +235,7 @@ TEST_F(JniTestWithNoDefaultJvmRef,
LocalObject<kClass1, kClassLoader, kJvm> a =
local_class_loader.BuildLocalObject<kClass1>();
LocalObject<kClass2> b{};
b("Foo", a);
b.Call<"Foo">(a);

TearDown();
}
Expand Down Expand Up @@ -310,7 +309,7 @@ TEST_F(JniTestWithNoDefaultJvmRef,
kDefaultConfiguration};
jni::LocalObject<class_under_test, class_loader, atypical_jvm_definition>
obj1{AdoptLocal{}, Fake<jobject>(1)};
obj1("Foo");
obj1.Call<"Foo">();

this->TearDown();
}
Expand Down
2 changes: 0 additions & 2 deletions implementation/class_loader_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include <gtest/gtest.h>
#include "jni_bind.h"
#include "jni_test.h"

namespace {

Expand Down
1 change: 0 additions & 1 deletion implementation/class_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "jni_bind.h"
#include "jni_test.h"
Expand Down
1 change: 1 addition & 0 deletions implementation/constructor_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "jni_bind.h"
#include "jni_test.h"
Expand Down
Loading

0 comments on commit 8348bd8

Please sign in to comment.