Review

Author: schmidt-sebastian

Firestore/core/src/model/object_value.cc

@@ -24,6 +24,7 @@
 #include "Firestore/core/src/nanopb/fields_array.h"
 #include "Firestore/core/src/nanopb/message.h"
 #include "Firestore/core/src/nanopb/nanopb_util.h"
+#include "Firestore/core/src/util/hashing.h"
 #include "absl/types/span.h"
 
 namespace firebase {
@@ -53,6 +54,7 @@ struct MapEntryKeyCompare {
   }
 };
 
+/** Traverses a Value proto and sorts all MapValues by key. */
 void SortFields(google_firestore_v1_Value& value) {
   if (value.which_value_type == google_firestore_v1_Value_map_value_tag) {
     google_firestore_v1_MapValue& map_value = value.map_value;
@@ -355,7 +357,7 @@ std::string ObjectValue::ToString() const {
 }
 
 size_t ObjectValue::Hash() const {
-  return std::hash<std::string>()(CanonicalId(*value_));
+  return util::Hash(CanonicalId(*value_));
 }
 
 /**

Firestore/core/src/model/object_value.h

@@ -45,12 +45,23 @@ class ObjectValue {
   explicit ObjectValue(const google_firestore_v1_Value& value);
 
   ObjectValue(ObjectValue&& other) noexcept = default;
-  ObjectValue& operator=(ObjectValue&& other) = default;
+  ObjectValue& operator=(ObjectValue&& other) noexcept = default;
   ObjectValue(const ObjectValue& other);
 
   ObjectValue& operator=(const ObjectValue&) = delete;
 
+  /**
+   * Creates a new ObjectValue that is backed by the given `map_value`.
+   * ObjectValue takes on ownership of the data.
+   */
   static ObjectValue FromMapValue(google_firestore_v1_MapValue map_value);
+
+  /**
+   * Creates a new ObjectValue that is backed by the provided document fields.
+   * ObjectValue takes on ownership of the data and zeroes out the pointers in
+   * `fields_entry`. This allows the callsite to destruct the Document proto
+   * without affecting the fields data.
+   */
   static ObjectValue FromFieldsEntry(
       google_firestore_v1_Document_FieldsEntry* fields_entry, pb_size_t count);
 

Firestore/core/test/unit/model/object_value_test.cc

@@ -48,13 +48,13 @@ TEST_F(ObjectValueTest, ExtractsFields) {
   ASSERT_EQ(google_firestore_v1_Value_map_value_tag,
             value.Get(Field("foo"))->which_value_type);
 
-  EXPECT_TRUE(Value(1) == *value.Get(Field("foo.a")));
-  EXPECT_TRUE(Value(true) == *value.Get(Field("foo.b")));
-  EXPECT_TRUE(Value("string") == *value.Get(Field("foo.c")));
+  EXPECT_EQ(Value(1), *value.Get(Field("foo.a")));
+  EXPECT_EQ(Value(true), *value.Get(Field("foo.b")));
+  EXPECT_EQ(Value("string"), *value.Get(Field("foo.c")));
 
-  EXPECT_TRUE(nullopt == value.Get(Field("foo.a.b")));
-  EXPECT_TRUE(nullopt == value.Get(Field("bar")));
-  EXPECT_TRUE(nullopt == value.Get(Field("bar.a")));
+  EXPECT_EQ(nullopt, value.Get(Field("foo.a.b")));
+  EXPECT_EQ(nullopt, value.Get(Field("bar")));
+  EXPECT_EQ(nullopt, value.Get(Field("bar.a")));
 }
 
 TEST_F(ObjectValueTest, ExtractsFieldMask) {