Allow Mojo structs as map keys

mojo/public/cpp/bindings/struct_ptr.h

Index: mojo/public/cpp/bindings/struct_ptr.h
diff --git a/mojo/public/cpp/bindings/struct_ptr.h b/mojo/public/cpp/bindings/struct_ptr.h
index 92f2728a3a08470541c0985e5d12e713e7363dbb..c5dad0c4d5979dfc6f253822ba99b3c48de8490d 100644
--- a/mojo/public/cpp/bindings/struct_ptr.h
+++ b/mojo/public/cpp/bindings/struct_ptr.h
@@ -5,15 +5,19 @@
 #ifndef MOJO_PUBLIC_CPP_BINDINGS_STRUCT_PTR_H_
 #define MOJO_PUBLIC_CPP_BINDINGS_STRUCT_PTR_H_
 
+#include <functional>
 #include <new>
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "mojo/public/cpp/bindings/lib/hash_util.h"
 #include "mojo/public/cpp/bindings/type_converter.h"
 
 namespace mojo {
 namespace internal {
 
+constexpr size_t kHashSeed = 31;
+
 template <typename Struct>
 class StructHelper {
  public:
@@ -78,28 +82,46 @@ class StructPtr {
   // that it contains Mojo handles).
   StructPtr Clone() const { return is_null() ? StructPtr() : ptr_->Clone(); }
 
+  // Compares the pointees (which might both be null).
   bool Equals(const StructPtr& other) const {
     if (is_null() || other.is_null())
       return is_null() && other.is_null();
     return ptr_->Equals(*other.ptr_);
   }
 
- private:
-  // TODO(dcheng): Use an explicit conversion operator.
-  typedef Struct* StructPtr::*Testable;
+  // Hashes based on the pointee (which might be null).
+  size_t Hash(size_t seed) const {
+    if (is_null())
+      return internal::HashCombine(seed, 0);
+    return ptr_->Hash(seed);
+  }
 
- public:
-  operator Testable() const { return ptr_ ? &StructPtr::ptr_ : 0; }
+  // For use in WTF::HashMap only:
+  bool isHashTableDeletedValue() const {
+    return ptr_ == reinterpret_cast<Struct*>(1u);
+  }
 
- private:
-  friend class internal::StructHelper<Struct>;
+  // For use in WTF::HashMap only:
+  static void constructDeletedValue(mojo::StructPtr<S>& slot) {
+    // Dirty trick: implant an invalid pointer in |ptr_|. Destructor isn't
+    // called for deleted buckets, so this is okay.
+    new (&slot) StructPtr();
+    slot.ptr_ = reinterpret_cast<Struct*>(1u);
+  }
 
-  // Forbid the == and != operators explicitly, otherwise StructPtr will be
-  // converted to Testable to do == or != comparison.
   template <typename T>
-  bool operator==(const StructPtr<T>& other) const = delete;
+  bool operator==(const StructPtr<T>& other) const {

[yzshen1, 2016/09/21 23:17:53]

Something to think about: If we decide to have "==" do value comparison, maybe
we should get rid of Equals entirely. WDYT?

[tibell, 2016/09/22 05:17:23]

I added a TODO. Both Sam and I want to get rid of it. It might be a sufficiently
large change to deserve its own CL though.

+    return this->Equals(other);
+  }
   template <typename T>
-  bool operator!=(const StructPtr<T>& other) const = delete;
+  bool operator!=(const StructPtr<T>& other) const {
+    return !(this->Equals(other));
+  }
+
+  explicit operator bool() const { return !is_null(); }
+
+ private:
+  friend class internal::StructHelper<Struct>;
 
   void Initialize() {
     DCHECK(!ptr_);
@@ -122,8 +144,8 @@ class InlinedStructPtr {
  public:
   using Struct = S;
 
-  InlinedStructPtr() : is_null_(true) {}
-  InlinedStructPtr(decltype(nullptr)) : is_null_(true) {}
+  InlinedStructPtr() : state_(NIL) {}
+  InlinedStructPtr(decltype(nullptr)) : state_(NIL) {}
 
   ~InlinedStructPtr() {}
 
@@ -132,7 +154,7 @@ class InlinedStructPtr {
     return *this;
   }
 
-  InlinedStructPtr(InlinedStructPtr&& other) : is_null_(true) { Take(&other); }
+  InlinedStructPtr(InlinedStructPtr&& other) : state_(NIL) { Take(&other); }
   InlinedStructPtr& operator=(InlinedStructPtr&& other) {
     Take(&other);
     return *this;
@@ -144,67 +166,106 @@ class InlinedStructPtr {
   }
 
   void reset() {
-    is_null_ = true;
+    state_ = NIL;
     value_. ~Struct();
     new (&value_) Struct();
   }
 
-  bool is_null() const { return is_null_; }
+  bool is_null() const { return state_ == NIL; }
 
   Struct& operator*() const {
-    DCHECK(!is_null_);
+    DCHECK(state_ == VALID);
     return value_;
   }
   Struct* operator->() const {
-    DCHECK(!is_null_);
+    DCHECK(state_ == VALID);
     return &value_;
   }
   Struct* get() const { return &value_; }
 
   void Swap(InlinedStructPtr* other) {
     std::swap(value_, other->value_);
-    std::swap(is_null_, other->is_null_);
+    std::swap(state_, other->state_);
   }
 
   InlinedStructPtr Clone() const {
     return is_null() ? InlinedStructPtr() : value_.Clone();
   }
+
+  // Compares the pointees (which might both be null).
   bool Equals(const InlinedStructPtr& other) const {
     if (is_null() || other.is_null())
       return is_null() && other.is_null();
     return value_.Equals(other.value_);
   }
 
- private:
-  // TODO(dcheng): Use an explicit conversion operator.
-  typedef Struct InlinedStructPtr::*Testable;
+  // Hashes based on the pointee (which might be null).
+  size_t Hash(size_t seed) const {
+    if (is_null())
+      return internal::HashCombine(seed, 0);
+    return value_.Hash(seed);
+  }
 
- public:
-  operator Testable() const { return is_null_ ? 0 : &InlinedStructPtr::value_; }
+  // For use in WTF::HashMap only:
+  bool isHashTableDeletedValue() const { return state_ == DELETED; }
 
- private:
-  friend class internal::StructHelper<Struct>;
+  // For use in WTF::HashMap only:
+  static void constructDeletedValue(mojo::InlinedStructPtr<S>& slot) {
+    new (&slot) InlinedStructPtr();

[yzshen1, 2016/09/21 23:17:52]

Is |slot|'s constructor already called before passed into this function? This
calls constructor for |slot| again without destructing it first.

(and also line 108)

[tibell, 2016/09/22 05:17:23]

|slot| referred to a previous, real value that got deleted and had its
destructor run, so this is the first time the "deleted value" has its
constructor called.

[yzshen1, 2016/09/22 23:48:56]

Please add a comment about this to the corresponding methods in
[Inlined]StructPtrWTFHelper.

+    slot.state_ = DELETED;
+  }
 
-  // Forbid the == and != operators explicitly, otherwise InlinedStructPtr will
-  // be converted to Testable to do == or != comparison.
   template <typename T>
-  bool operator==(const InlinedStructPtr<T>& other) const = delete;
+  bool operator==(const InlinedStructPtr<T>& other) const {
+    return this->Equals(other);
+  }
   template <typename T>
-  bool operator!=(const InlinedStructPtr<T>& other) const = delete;
+  bool operator!=(const InlinedStructPtr<T>& other) const {
+    return !(this->Equals(other));
+  }
+
+  explicit operator bool() const { return !is_null(); }
+
+ private:
+  friend class internal::StructHelper<Struct>;
 
-  void Initialize() { is_null_ = false; }
+  void Initialize() { state_ = VALID; }
 
   void Take(InlinedStructPtr* other) {
     reset();
     Swap(other);
   }
 
+  enum State {
+    VALID,
+    NIL,
+    DELETED,  // For use in WTF::HashMap only
+  };
+
   mutable Struct value_;
-  bool is_null_;
+  State state_;
 
   DISALLOW_COPY_AND_ASSIGN(InlinedStructPtr);
 };
 
 }  // namespace mojo
 
+namespace std {
+
+template <typename T>
+struct hash<mojo::StructPtr<T>> {
+  size_t operator()(const mojo::StructPtr<T>& value) const {
+    return value.Hash(mojo::internal::kHashSeed);
+  }
+};
+
+template <typename T>
+struct hash<mojo::InlinedStructPtr<T>> {
+  size_t operator()(const mojo::InlinedStructPtr<T>& value) const {
+    return value.Hash(mojo::internal::kHashSeed);
+  }
+};
+
+}  // namespace std
+
 #endif  // MOJO_PUBLIC_CPP_BINDINGS_STRUCT_PTR_H_