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 · 254207a · 254207a
1 parent acce094
commit 254207a
Show file tree

Hide file tree

Showing 11 changed files with 210 additions and 130 deletions.
diff --git a/implementation/array.h b/implementation/array.h
@@ -52,20 +52,29 @@ 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;
+////////////////////////////////////////////////////////////////////////////////
+// Equality operators.
+////////////////////////////////////////////////////////////////////////////////
+template <typename RawTypeLhs, std::size_t kRankLhs, typename RawTypeRhs,
+          std::size_t kRankRhs>
+constexpr bool operator==(
+    const ArrayNonObjectTypeImpl<RawTypeLhs, kRankLhs>& lhs,
+    const Array<RawTypeRhs, kRankRhs>& rhs) {
+  if constexpr (std::is_same_v<RawTypeLhs, RawTypeRhs>) {
+    return (lhs.raw_ == rhs.raw_);
   }
+  return false;
+}
 
-  template <typename RawTypeRhs, std::size_t kRankRhs>
-  constexpr bool operator!=(const Array<RawTypeRhs, kRankRhs>& rhs) const {
-    return !(*this == rhs);
-  }
-};
+template <typename RawTypeLhs, std::size_t kRankLhs, typename RawTypeRhs,
+          std::size_t kRankRhs>
+constexpr bool operator!=(
+    const ArrayNonObjectTypeImpl<RawTypeLhs, kRankLhs>& lhs,
+    const Array<RawTypeRhs, kRankRhs>& rhs) {
+  return !(lhs == rhs);
+}
 
 // Primitive array implementaiton.
 template <typename T, std::size_t kRank, bool HoldsObject>
@@ -86,20 +95,24 @@ struct ArrayImpl<RawType, kRank_, true> : public ArrayTag<jobjectArray> {
 
   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/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) {

diff --git a/implementation/constructor.h b/implementation/constructor.h
@@ -19,10 +19,12 @@
 
 // IWYU pragma: private, include "third_party/jni_wrapper/jni_bind.h"
 
-#include "method.h"
-#include "params.h"
-#include "return.h"
+#include <cstddef>
+#include <string>
+#include <type_traits>
+
 #include "implementation/jni_helper/jni_helper.h"
+#include "implementation/params.h"
 
 namespace jni {
 
@@ -52,15 +54,15 @@ class Constructor : public ConstructorBase {
   constexpr explicit Constructor(Args...) {}
 };
 
-template <typename... ParamsRaw>
-Constructor(ParamsRaw...) -> Constructor<ParamsRaw...>;
-
 template <typename... LhsParams, typename... RhsParams>
 constexpr bool operator==(const Constructor<LhsParams...>& lhs,
                           const Constructor<RhsParams...>& rhs) {
   return lhs.params_ == rhs.params_;
 }
 
+template <typename... ParamsRaw>
+Constructor(ParamsRaw...) -> Constructor<ParamsRaw...>;
+
 //==============================================================================
 // Represents a constructor used at runtime and has index data about where it
 // exists in the static class definition which is embedded on the caller's

diff --git a/implementation/default_class_loader.h b/implementation/default_class_loader.h
@@ -62,18 +62,21 @@ class DefaultClassLoader {
   constexpr std::size_t IdxOfAncestor(std::size_t cur_idx = 0) const {
     return kClassNotInLoaderSetIdx;
   }
+};
 
-  template <typename T>
-  bool constexpr operator==(const T& rhs) const {
-    return false;
-  }
-  bool constexpr operator==(const DefaultClassLoader&) const { return true; }
+template <typename T>
+bool constexpr operator==(const DefaultClassLoader& lhs, const T& rhs) {
+  return false;
+}
+bool constexpr operator==(const DefaultClassLoader& lhs,
+                          const DefaultClassLoader& rhs) {
+  return true;
+}
 
-  template <typename T>
-  bool constexpr operator!=(const T& rhs) const {
-    return !(*this == rhs);
-  }
-};
+template <typename T>
+bool constexpr operator!=(const DefaultClassLoader& lhs, const T& rhs) {
+  return !(lhs == rhs);
+}
 
 // Class loader that cannot supply any classes. This should be the root loader
 // for most user defined classes.
@@ -95,19 +98,22 @@ class NullClassLoader {
   constexpr std::size_t IdxOfAncestor(std::size_t cur_idx = 0) const {
     return kClassNotInLoaderSetIdx;
   }
-
-  template <typename T>
-  bool constexpr operator==(const T& rhs) const {
-    return false;
-  }
-  bool constexpr operator==(const NullClassLoader&) const { return true; }
-
-  template <typename T>
-  bool constexpr operator!=(const T& rhs) const {
-    return !(*this == rhs);
-  }
 };
 
+template <typename T>
+constexpr bool operator==(const NullClassLoader& lhs, const T& rhs) {
+  return false;
+}
+constexpr bool operator==(const NullClassLoader& lhs,
+                          const NullClassLoader& rhs) {
+  return true;
+}
+
+template <typename T>
+constexpr bool operator!=(const NullClassLoader& lhs, const T& rhs) {
+  return !(lhs == rhs);
+}
+
 static constexpr NullClassLoader kNullClassLoader;
 static constexpr DefaultClassLoader kDefaultClassLoader;
 

diff --git a/implementation/jvm.h b/implementation/jvm.h
@@ -50,18 +50,23 @@ class Jvm {
     return IdxOfClassLoaderHelper<class_loader_v>(
         std::make_integer_sequence<std::size_t, sizeof...(ClassLoaderTs)>());
   }
+};
 
-  template <typename T>
-  bool constexpr operator==(const T& rhs) const {
-    return false;
-  }
-  bool constexpr operator==(const Jvm&) const { return true; }
+template <typename... ClassLoaderTs, typename T>
+constexpr bool operator==(const Jvm<ClassLoaderTs...>& lhs, const T& rhs) {
+  return false;
+}
 
-  template <typename T>
-  bool constexpr operator!=(const T& rhs) const {
-    return !(*this == rhs);
-  }
-};
+template <typename... ClassLoaderLhsTs>
+constexpr bool operator==(const Jvm<ClassLoaderLhsTs...>& lhs,
+                          const Jvm<ClassLoaderLhsTs...>& rhs) {
+  return true;
+}
+
+template <typename... ClassLoaderTs, typename T>
+bool constexpr operator!=(const Jvm<ClassLoaderTs...>& lhs, const T& rhs) {
+  return !(lhs == rhs);
+}
 
 template <typename... ClassLoaderTs>
 Jvm(ClassLoaderTs...) -> Jvm<ClassLoaderTs...>;