Fixing ambiguous equality operators for C++20 migration.

This is an issue specifically on CRTP classes templated equality operators that is resolved by making equality operators free standing functions. e.g. ``` implementation/local_object_test.cc:194:22: error: use of overloaded operator '==' is ambiguous (with operand types 'LocalObject<kClass>' and 'LocalObject<kClass2>') EXPECT_FALSE(val_1 == val_2); ~~~~~ ^ ~~~~~ ``` See #346. Sample failure https://github.com/google/jni-bind/actions/runs/11474529819/job/31930608615?pr=345. PiperOrigin-RevId: 688985785
google · Oct 23, 2024 · 7c283af · 7c283af
1 parent acce094
commit 7c283af
Show file tree

Hide file tree

Showing 21 changed files with 311 additions and 174 deletions.
diff --git a/implementation/BUILD b/implementation/BUILD
@@ -83,6 +83,7 @@ cc_library(
     deps = [
         ":array_type_conversion",
         "//:jni_dep",
+        "//implementation/jni_helper:get_array_element_result",
         "//implementation/jni_helper:jni_array_helper",
         "//implementation/jni_helper:lifecycle",
     ],
@@ -296,23 +297,19 @@ cc_library(
     name = "field_ref",
     hdrs = ["field_ref.h"],
     deps = [
-        ":class_ref",
+        ":default_class_loader",
         ":field_selection",
         ":id",
         ":id_type",
         ":no_idx",
         ":promotion_mechanics_tags",
-        ":proxy",
         ":proxy_convenience_aliases",
         ":ref_base",
         ":signature",
         "//:jni_dep",
         "//implementation/jni_helper",
         "//implementation/jni_helper:field_value_getter",
-        "//implementation/jni_helper:static_field_value",
         "//metaprogramming:double_locked_value",
-        "//metaprogramming:optional_wrap",
-        "//metaprogramming:queryable_map",
     ],
 )
 
@@ -365,10 +362,8 @@ cc_library(
     name = "forward_declarations",
     hdrs = ["forward_declarations.h"],
     deps = [
-        ":default_class_loader",
         ":id_type",
-        ":jvm",
-        "//implementation/jni_helper:lifecycle_object",
+        "//implementation/jni_helper:lifecycle",
     ],
 )
 
@@ -397,8 +392,10 @@ cc_library(
     name = "global_object",
     hdrs = ["global_object.h"],
     deps = [
+        ":default_class_loader",
         ":forward_declarations",
         ":jni_type",
+        ":jvm",
         ":local_object",
         ":promotion_mechanics",
         ":ref_base",
@@ -724,6 +721,7 @@ cc_library(
         ":forward_declarations",
         ":jvm",
         ":promotion_mechanics_tags",
+        "//:jni_dep",
         "//implementation/jni_helper:lifecycle",
     ],
 )

diff --git a/implementation/array.h b/implementation/array.h
@@ -34,11 +34,13 @@ struct Array;
 template <std::size_t kRank>
 struct Rank {};
 
+struct ArrayBase {};
+
 ////////////////////////////////////////////////////////////////////////////////
 // Array Non-Object Implementation.
 ////////////////////////////////////////////////////////////////////////////////
 template <typename RawType, std::size_t kRank>
-struct ArrayNonObjectTypeImpl {
+struct ArrayNonObjectTypeImpl : ArrayBase {
   RawType raw_;
 
   constexpr ArrayNonObjectTypeImpl(RawType raw) : raw_(raw) {}
@@ -52,21 +54,48 @@ struct ArrayNonObjectTypeImpl {
                   "JNI Error: Invalid array declaration, use Array { type{}, "
                   "Rank<kRank>{} }.");
   }
+};
 
-  template <typename RawTypeRhs, std::size_t kRankRhs>
-  constexpr bool operator==(const Array<RawTypeRhs, kRankRhs>& rhs) const {
-    if constexpr (std::is_same_v<RawType, RawTypeRhs>) {
-      return (raw_ == rhs.raw_);
-    }
-    return false;
-  }
+template <typename T>
+struct ArrayComparisonHelper {};
 
-  template <typename RawTypeRhs, std::size_t kRankRhs>
-  constexpr bool operator!=(const Array<RawTypeRhs, kRankRhs>& rhs) const {
-    return !(*this == rhs);
-  }
+template <typename RawType, std::size_t kRank>
+struct ArrayComparisonHelper<Array<ArrayNonObjectTypeImpl<RawType, kRank>>> {
+  using type = RawType;
+};
+
+template <typename RawType, std::size_t kRank>
+struct ArrayComparisonHelper<Array<RawType, kRank>> {
+  using type = RawType;
+};
+
+template <typename T>
+using ArrayComparisonHelper_t = typename ArrayComparisonHelper<T>::type;
+
+template <typename T1, typename T2>
+static constexpr bool IsArrayComparable() {
+  return std::is_base_of_v<ArrayBase, T1> && std::is_base_of_v<ArrayBase, T2>;
 };
 
+////////////////////////////////////////////////////////////////////////////////
+// Equality operators.
+////////////////////////////////////////////////////////////////////////////////
+template <typename T1, typename T2>
+constexpr std::enable_if_t<IsArrayComparable<T1, T2>(), bool> operator==(
+    const T1& lhs, const T2& rhs) {
+  if constexpr (std::is_same_v<ArrayComparisonHelper_t<T1>,
+                               ArrayComparisonHelper_t<T2>>) {
+    return (lhs.raw_ == rhs.raw_);
+  }
+  return false;
+}
+
+template <typename T1, typename T2>
+constexpr std::enable_if_t<IsArrayComparable<T1, T2>(), bool> operator!=(
+    const T1& lhs, const T2& rhs) {
+  return !(lhs == rhs);
+}
+
 // Primitive array implementaiton.
 template <typename T, std::size_t kRank, bool HoldsObject>
 struct ArrayImpl : public ArrayNonObjectTypeImpl<T, kRank>,
@@ -79,27 +108,32 @@ struct ArrayImpl : public ArrayNonObjectTypeImpl<T, kRank>,
 // Array Object Implementation.
 ////////////////////////////////////////////////////////////////////////////////
 template <typename RawType, std::size_t kRank_>
-struct ArrayImpl<RawType, kRank_, true> : public ArrayTag<jobjectArray> {
+struct ArrayImpl<RawType, kRank_, true> : public ArrayTag<jobjectArray>,
+                                          ArrayBase {
   RawType raw_;
 
   constexpr ArrayImpl(RawType raw) : raw_(raw) {}
 
   template <std::size_t kRank>
   constexpr ArrayImpl(RawType raw, Rank<kRank>) : raw_(raw) {}
+};
 
-  template <typename RawTypeRhs, std::size_t kRank>
-  constexpr bool operator==(const Array<RawTypeRhs, kRank>& rhs) const {
-    if constexpr (std::is_same_v<RawType, RawTypeRhs>) {
-      return (raw_ == rhs.raw_);
-    }
-    return false;
+template <typename RawTypeLhs, std::size_t kRankLhs, typename RawTypeRhs,
+          std::size_t kRankRhs>
+constexpr bool operator==(const ArrayImpl<RawTypeLhs, kRankLhs, true>& lhs,
+                          const ArrayImpl<RawTypeRhs, kRankRhs, true>& rhs) {
+  if constexpr (std::is_same_v<RawTypeLhs, RawTypeRhs>) {
+    return (lhs.raw_ == rhs.raw_);
   }
+  return false;
+}
 
-  template <typename RawTypeRhs, std::size_t kRank>
-  constexpr bool operator!=(const Array<RawTypeRhs, kRank>& rhs) const {
-    return !(*this == rhs);
-  }
-};
+template <typename RawTypeLhs, std::size_t kRankLhs, typename RawTypeRhs,
+          std::size_t kRankRhs>
+constexpr bool operator!=(const ArrayImpl<RawTypeLhs, kRankLhs, true>& lhs,
+                          const Array<RawTypeRhs, kRankRhs>& rhs) {
+  return !(lhs == rhs);
+}
 
 // This type correlates to those used in declarations,
 //   e.g. Field { Array { Array { jint {} } } }.

diff --git a/implementation/array_test.cc b/implementation/array_test.cc
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#include <type_traits>
+
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include "jni_bind.h"
@@ -108,5 +110,4 @@ static_assert(
 static_assert(FullArrayStripV(arr8) == kClass);
 static_assert(FullArrayStripV(arr9) == kClass);
 
-
 }  // namespace
diff --git a/implementation/array_view.h b/implementation/array_view.h
@@ -21,26 +21,33 @@
 
 #include <cstddef>
 #include <iterator>
+#include <type_traits>
 
 #include "implementation/array_type_conversion.h"
+#include "implementation/jni_helper/get_array_element_result.h"
 #include "implementation/jni_helper/jni_array_helper.h"
 #include "implementation/jni_helper/lifecycle.h"
 #include "jni_dep.h"
 
 namespace jni {
 
+struct ArrayViewHelperBase {};
+
 template <typename T>
-struct ArrayViewHelper {
+struct ArrayViewHelper : ArrayViewHelperBase {
+  using SpanType = T;
   const T val_;
   operator T() const { return val_; }
 
   ArrayViewHelper(const T& val) : val_(val) {}
 };
 
 // Primitive Rank 1 Arrays.
-template <typename SpanType, std::size_t kRank = 1, typename Enable = void>
+template <typename SpanType_, std::size_t kRank = 1, typename Enable = void>
 class ArrayView {
  public:
+  using SpanType = SpanType_;
+
   struct Iterator {
     using iterator_category = std::random_access_iterator_tag;
     using difference_type = std::size_t;
@@ -115,12 +122,14 @@ class ArrayView {
 };
 
 // Object arrays, or arrays with rank > 1 (which are object arrays), or strings.
-template <typename SpanType, std::size_t kRank>
+template <typename SpanType_, std::size_t kRank>
 class ArrayView<
-    SpanType, kRank,
-    std::enable_if_t<(kRank > 1) || std::is_same_v<SpanType, jobject> ||
-                     std::is_same_v<SpanType, jstring>>> {
+    SpanType_, kRank,
+    std::enable_if_t<(kRank > 1) || std::is_same_v<SpanType_, jobject> ||
+                     std::is_same_v<SpanType_, jstring>>> {
  public:
+  using SpanType = SpanType_;
+
   // Metafunction that returns the type after a single dereference.
   template <std::size_t>
   struct PinHelper {

diff --git a/implementation/class.h b/implementation/class.h
@@ -156,27 +156,45 @@ struct Class<Extends_, std::tuple<Constructors_...>,
         static_(statik),
         methods_(methods...),
         fields_(fields...) {}
-
-  ////////////////////////////////////////////////////////////////////////////////
-  // Equality operators.
-  ////////////////////////////////////////////////////////////////////////////////
-  template <typename ParentClass, typename... Params, typename... Constructors,
-            typename... StaticMethods, typename... StaticFields,
-            typename... Fields, typename... Methods>
-  constexpr bool operator==(
-      const Class<ParentClass, std::tuple<Constructors...>,
-                  std::tuple<Static<std::tuple<StaticMethods...>,
-                                    std::tuple<StaticFields...>>>,
-                  std::tuple<Methods...>, std::tuple<Fields...>>& rhs) const {
-    // Don't compare the other parameters so classes can be used as parameters
-    // or return values before the class itself is defined.
-    return std::string_view(name_) == std::string_view(rhs.name_);
-  }
-
-  constexpr bool operator==(const NoClass&) const { return false; }
-  constexpr bool operator!=(const NoClass&) const { return false; }
 };
 
+////////////////////////////////////////////////////////////////////////////////
+// Equality operators.
+////////////////////////////////////////////////////////////////////////////////
+template <typename LhsParentClass, typename... LhsParams,
+          typename... LhsConstructors, typename... LhsStaticMethods,
+          typename... LhsStaticFields, typename... LhsFields,
+          typename... LhsMethods, typename RhsParentClass,
+          typename... RhsParams, typename... RhsConstructors,
+          typename... RhsStaticMethods, typename... RhsStaticFields,
+          typename... RhsFields, typename... RhsMethods>
+constexpr bool operator==(
+    const Class<LhsParentClass, std::tuple<LhsConstructors...>,
+                std::tuple<Static<std::tuple<LhsStaticMethods...>,
+                                  std::tuple<LhsStaticFields...>>>,
+                std::tuple<LhsMethods...>, std::tuple<LhsFields...>>& lhs,
+    const Class<RhsParentClass, std::tuple<RhsConstructors...>,
+                std::tuple<Static<std::tuple<RhsStaticMethods...>,
+                                  std::tuple<RhsStaticFields...>>>,
+                std::tuple<RhsMethods...>, std::tuple<RhsFields...>>& rhs) {
+  // Don't compare the other parameters so classes can be used as parameters
+  // or return values before the class itself is defined.
+  return std::string_view(lhs.name_) == std::string_view(rhs.name_);
+}
+
+template <typename... Ts>
+constexpr bool operator==(const Class<Ts...>& lhs, const NoClass&) {
+  return false;
+}
+
+template <typename... Ts>
+constexpr bool operator!=(const Class<Ts...>& lhs, const NoClass&) {
+  return true;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// CTAD.
+////////////////////////////////////////////////////////////////////////////////
 template <typename... Params>
 Class(const char*, Params...)
     -> Class<metaprogramming::BaseFilterWithDefault_t<

diff --git a/implementation/class_loader.h b/implementation/class_loader.h
@@ -19,6 +19,7 @@
 
 // IWYU pragma: private, include "third_party/jni_wrapper/jni_bind.h"
 
+#include <cstddef>
 #include <tuple>
 #include <type_traits>
 #include <utility>
@@ -68,20 +69,6 @@ class ClassLoader : public Object {
         parent_loader_(parent_loader),
         supported_classes_(supported_class_set.supported_classes_) {}
 
-  bool constexpr operator==(
-      const ClassLoader<ParentLoader_, SupportedClasses_...>& rhs) const {
-    return (*this).parent_loader_ == rhs.parent_loader_ &&
-           (*this).supported_classes_ == rhs.supported_classes_;
-  }
-  template <typename T>
-  bool constexpr operator==(const T& rhs) const {
-    return false;
-  }
-  template <typename T>
-  bool constexpr operator!=(const T& rhs) const {
-    return !(*this == rhs);
-  }
-
   template <const auto& class_v, std::size_t... Is>
   constexpr std::size_t IdxOfClassHelper(
       std::integer_sequence<std::size_t, Is...>) const {
@@ -131,6 +118,33 @@ class ClassLoader : public Object {
   }
 };
 
+////////////////////////////////////////////////////////////////////////////////
+// Equality operators.
+////////////////////////////////////////////////////////////////////////////////
+template <typename ParentLoader_, typename... SupportedClasses_>
+bool constexpr operator==(
+    const ClassLoader<ParentLoader_, SupportedClasses_...>& lhs,
+    const ClassLoader<ParentLoader_, SupportedClasses_...>& rhs) {
+  return lhs.parent_loader_ == rhs.parent_loader_ &&
+         lhs.supported_classes_ == rhs.supported_classes_;
+}
+
+template <typename ParentLoader_, typename... SupportedClasses_, typename T>
+bool constexpr operator==(
+    const ClassLoader<ParentLoader_, SupportedClasses_...>& lhs, const T& rhs) {
+  return false;
+}
+
+template <typename ParentLoader_, typename... SupportedClasses_, typename T>
+bool constexpr operator!=(
+    const ClassLoader<ParentLoader_, SupportedClasses_...>& lhs, const T& rhs) {
+  return !(lhs == rhs);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// CTAD.
+////////////////////////////////////////////////////////////////////////////////
+
 // Note: Null is chosen, not default, because LoadedBy requires a syntax like
 // LoadedBy{ClassLoader{"kClass"}} (using the CTAD loader type below), but
 // we want to prevent explicit usage of a default loader (as it makes no sense).
@@ -146,6 +160,9 @@ template <typename... SupportedClasses_>
 ClassLoader(SupportedClassSet<SupportedClasses_...>)
     -> ClassLoader<DefaultClassLoader, SupportedClasses_...>;
 
+////////////////////////////////////////////////////////////////////////////////
+// Ancestral lookups.
+////////////////////////////////////////////////////////////////////////////////
 template <typename T, std::size_t I>
 constexpr auto& GetAncestor(const T& loader) {
   if constexpr (I == 0) {