[Mojo] Remove dependency on Hash() in (Inlined)StructPtr::operator<

This change removes the dependency on Hash() in the implementation of
StructPtr's and InlinedStructPtr's operator<, and replaces it with a deep
comparison.

Bug: 735302, 911929
Change-Id: I2d81d1990787e29a1588f3783eb91d0eaa98207b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1665934Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671867}

mojo/public/cpp/bindings/struct_ptr.h

@@ -99,10 +99,6 @@ class StructPtr {
 
   explicit operator bool() const { return !is_null(); }
 
-  bool operator<(const StructPtr& other) const {
-    return Hash(internal::kHashSeed) < other.Hash(internal::kHashSeed);
-  }
-
  private:
   friend class internal::StructPtrWTFHelper<Struct>;
   void Take(StructPtr* other) {
@@ -115,32 +111,23 @@ class StructPtr {
   DISALLOW_COPY_AND_ASSIGN(StructPtr);
 };
 
-template <typename T>
-bool operator==(const StructPtr<T>& lhs, const StructPtr<T>& rhs) {
-  return lhs.Equals(rhs);
-}
-template <typename T>
-bool operator!=(const StructPtr<T>& lhs, const StructPtr<T>& rhs) {
-  return !(lhs == rhs);
-}
-
 // Designed to be used when Struct is small and copyable.
 template <typename S>
 class InlinedStructPtr {
  public:
   using Struct = S;
 
-  InlinedStructPtr() : state_(NIL) {}
-  InlinedStructPtr(std::nullptr_t) : state_(NIL) {}
+  InlinedStructPtr() = default;
+  InlinedStructPtr(std::nullptr_t) {}
 
-  ~InlinedStructPtr() {}
+  ~InlinedStructPtr() = default;
 
   InlinedStructPtr& operator=(std::nullptr_t) {
     reset();
     return *this;
   }
 
-  InlinedStructPtr(InlinedStructPtr&& other) : state_(NIL) { Take(&other); }
+  InlinedStructPtr(InlinedStructPtr&& other) { Take(&other); }
   InlinedStructPtr& operator=(InlinedStructPtr&& other) {
     Take(&other);
     return *this;
@@ -198,10 +185,6 @@ class InlinedStructPtr {
 
   explicit operator bool() const { return !is_null(); }
 
-  bool operator<(const InlinedStructPtr& other) const {
-    return Hash(internal::kHashSeed) < other.Hash(internal::kHashSeed);
-  }
-
  private:
   friend class internal::InlinedStructPtrWTFHelper<Struct>;
   void Take(InlinedStructPtr* other) {
@@ -210,28 +193,17 @@ class InlinedStructPtr {
   }
 
   enum State {
-    VALID,
     NIL,
+    VALID,
     DELETED,  // For use in WTF::HashMap only
   };
 
   mutable Struct value_;
-  State state_;
+  State state_ = NIL;
 
   DISALLOW_COPY_AND_ASSIGN(InlinedStructPtr);
 };
 
-template <typename T>
-bool operator==(const InlinedStructPtr<T>& lhs,
-                const InlinedStructPtr<T>& rhs) {
-  return lhs.Equals(rhs);
-}
-template <typename T>
-bool operator!=(const InlinedStructPtr<T>& lhs,
-                const InlinedStructPtr<T>& rhs) {
-  return !(lhs == rhs);
-}
-
 namespace internal {
 
 template <typename Struct>
@@ -269,7 +241,56 @@ class InlinedStructPtrWTFHelper {
   }
 };
 
+// Convenience type trait so that we can get away with defining the comparison
+// operators only once.
+template <typename T>
+struct IsStructPtrImpl : std::false_type {};
+
+template <typename S>
+struct IsStructPtrImpl<StructPtr<S>> : std::true_type {};
+
+template <typename S>
+struct IsStructPtrImpl<InlinedStructPtr<S>> : std::true_type {};
+
 }  // namespace internal
+
+template <typename T>
+constexpr bool IsStructPtrV = internal::IsStructPtrImpl<std::decay_t<T>>::value;
+
+template <typename Ptr, std::enable_if_t<IsStructPtrV<Ptr>>* = nullptr>
+bool operator==(const Ptr& lhs, const Ptr& rhs) {
+  return lhs.Equals(rhs);
+}
+
+template <typename Ptr, std::enable_if_t<IsStructPtrV<Ptr>>* = nullptr>
+bool operator!=(const Ptr& lhs, const Ptr& rhs) {
+  return !(lhs == rhs);
+}
+
+// Perform a deep comparison if possible. Otherwise treat null pointers less
+// than valid pointers.
+template <typename Ptr, std::enable_if_t<IsStructPtrV<Ptr>>* = nullptr>
+bool operator<(const Ptr& lhs, const Ptr& rhs) {
+  if (!lhs || !rhs)
+    return bool{lhs} < bool{rhs};
+  return *lhs < *rhs;
+}
+
+template <typename Ptr, std::enable_if_t<IsStructPtrV<Ptr>>* = nullptr>
+bool operator<=(const Ptr& lhs, const Ptr& rhs) {
+  return !(rhs < lhs);
+}
+
+template <typename Ptr, std::enable_if_t<IsStructPtrV<Ptr>>* = nullptr>
+bool operator>(const Ptr& lhs, const Ptr& rhs) {
+  return rhs < lhs;
+}
+
+template <typename Ptr, std::enable_if_t<IsStructPtrV<Ptr>>* = nullptr>
+bool operator>=(const Ptr& lhs, const Ptr& rhs) {
+  return !(lhs < rhs);
+}
+
 }  // namespace mojo
 
 namespace std {

mojo/public/cpp/bindings/tests/rect_chromium.h

@@ -57,12 +57,17 @@ class RectChromium {
 
   int GetArea() const { return width_ * height_; }
 
+  auto TieForCmp() const { return std::tie(x_, y_, width_, height_); }
+
   bool operator==(const RectChromium& other) const {
-    return (x() == other.x() && y() == other.y() && width() == other.width() &&
-            height() == other.height());
+    return TieForCmp() == other.TieForCmp();
   }
   bool operator!=(const RectChromium& other) const { return !(*this == other); }
 
+  bool operator<(const RectChromium& other) const {
+    return TieForCmp() < other.TieForCmp();
+  }
+
  private:
   int x_ = 0;
   int y_ = 0;

mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl

@@ -3,6 +3,8 @@
 {{ kythe_annotation(struct_prefix) }}
 class {{export_attribute}} {{struct.name}} {
  public:
+  template <typename T>
+  using EnableIfSame = std::enable_if_t<std::is_same<{{struct.name}}, T>::value>;
   using DataView = {{struct.name}}DataView;
   using Data_ = internal::{{struct.name}}_Data;
 
@@ -54,9 +56,7 @@ class {{export_attribute}} {{struct.name}} {
   // Equals() is a template so it is only instantiated if it is used. Thus, the
   // bindings generator does not need to know whether Equals() or == operator
   // are available for members.
-  template <typename T,
-            typename std::enable_if<std::is_same<
-                T, {{struct.name}}>::value>::type* = nullptr>
+  template <typename T, {{struct.name}}::EnableIfSame<T>* = nullptr>
   bool Equals(const T& other) const;
 
 {%- if struct|is_hashable %}
@@ -136,3 +136,24 @@ class {{export_attribute}} {{struct.name}} {
   DISALLOW_COPY_AND_ASSIGN({{struct.name}});
 {%- endif %}
 };
+
+// The comparison operators are templates, so they are only instantiated if they
+// are used. Thus, the bindings generator does not need to know whether
+// comparison operators are available for members.
+template <typename T, {{struct.name}}::EnableIfSame<T>* = nullptr>
+bool operator<(const T& lhs, const T& rhs);
+
+template <typename T, {{struct.name}}::EnableIfSame<T>* = nullptr>
+bool operator<=(const T& lhs, const T& rhs) {
+  return !(rhs < lhs);
+}
+
+template <typename T, {{struct.name}}::EnableIfSame<T>* = nullptr>
+bool operator>(const T& lhs, const T& rhs) {
+  return rhs < lhs;
+}
+
+template <typename T, {{struct.name}}::EnableIfSame<T>* = nullptr>
+bool operator>=(const T& lhs, const T& rhs) {
+  return !(lhs < rhs);
+}

mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_template_definition.tmpl

@@ -8,9 +8,7 @@ template <typename StructPtrType>
   );
 }
 
-template <typename T,
-          typename std::enable_if<std::is_same<
-              T, {{struct.name}}>::value>::type*>
+template <typename T, {{struct.name}}::EnableIfSame<T>*>
 bool {{struct.name}}::Equals(const T& other_struct) const {
 {%- for field in struct.fields %}
   if (!mojo::Equals(this->{{field.name}}, other_struct.{{field.name}}))
@@ -18,3 +16,14 @@ bool {{struct.name}}::Equals(const T& other_struct) const {
 {%- endfor %}
   return true;
 }
+
+template <typename T, {{struct.name}}::EnableIfSame<T>*>
+bool operator<(const T& lhs, const T& rhs) {
+{%- for field in struct.fields %}
+  if (lhs.{{field.name}} < rhs.{{field.name}})
+    return true;
+  if (rhs.{{field.name}} < lhs.{{field.name}})
+    return false;
+{%- endfor %}
+  return false;
+}