Feedback

Author: schmidt-sebastian

firestore/src/main/document_snapshot_main.cc

@@ -110,7 +110,7 @@ FieldValue DocumentSnapshotInternal::ConvertObject(
   for (pb_size_t i = 0; i < object.fields_count; ++i) {
     std::string key = MakeString(object.fields[i].key);
     const google_firestore_v1_Value& value = object.fields[i].value;
-    result[key] = ConvertAnyValue(value, stb);
+    result[std::move(key)] = ConvertAnyValue(value, stb);
   }
 
   return FieldValue::Map(std::move(result));
@@ -174,7 +174,7 @@ FieldValue DocumentSnapshotInternal::ConvertReference(
   SIMPLE_HARD_ASSERT(database_id == firestore_internal()->database_id(),
                      "Converted reference is from another database");
 
-  api::DocumentReference api_reference{key, snapshot_.firestore()};
+  api::DocumentReference api_reference{std::move(key), snapshot_.firestore()};
   return FieldValue::Reference(MakePublic(std::move(api_reference)));
 }
 

firestore/src/main/field_value_main.cc

@@ -30,41 +30,41 @@ using Type = FieldValue::Type;
 // Constructors
 
 FieldValueInternal::FieldValueInternal(bool value) : type_{Type::kBoolean} {
-  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+  auto& proto = GetProtoValue();
   proto->which_value_type = google_firestore_v1_Value_boolean_value_tag;
   proto->boolean_value = value;
 }
 
 FieldValueInternal::FieldValueInternal(int64_t value) : type_{Type::kInteger} {
-  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+  auto& proto = GetProtoValue();
   proto->which_value_type = google_firestore_v1_Value_integer_value_tag;
   proto->integer_value = value;
 }
 
 FieldValueInternal::FieldValueInternal(double value) : type_{Type::kDouble} {
-  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+  auto& proto = GetProtoValue();
   proto->which_value_type = google_firestore_v1_Value_double_value_tag;
   proto->double_value = value;
 }
 
 FieldValueInternal::FieldValueInternal(Timestamp value)
     : type_{Type::kTimestamp} {
-  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+  auto& proto = GetProtoValue();
   proto->which_value_type = google_firestore_v1_Value_timestamp_value_tag;
   proto->timestamp_value.seconds = value.seconds();
   proto->timestamp_value.nanos = value.nanoseconds();
 }
 
 FieldValueInternal::FieldValueInternal(std::string value)
     : type_{Type::kString} {
-  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+  auto& proto = GetProtoValue();
   proto->which_value_type = google_firestore_v1_Value_string_value_tag;
   proto->string_value = MakeBytesArray(value);
 }
 
 FieldValueInternal::FieldValueInternal(const uint8_t* value, size_t size)
     : type_{Type::kBlob} {
-  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+  auto& proto = GetProtoValue();
   proto->which_value_type = google_firestore_v1_Value_bytes_value_tag;
   proto->bytes_value = MakeBytesArray(value, size);
 }
@@ -74,7 +74,7 @@ FieldValueInternal::FieldValueInternal(DocumentReference value)
 
 FieldValueInternal::FieldValueInternal(GeoPoint value)
     : type_{Type::kGeoPoint} {
-  auto proto = absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+  auto& proto = GetProtoValue();
   proto->which_value_type = google_firestore_v1_Value_geo_point_value_tag;
   proto->geo_point_value.latitude = value.latitude();
   proto->geo_point_value.longitude = value.longitude();
@@ -90,47 +90,39 @@ FieldValueInternal::FieldValueInternal(MapFieldValue value)
 
 bool FieldValueInternal::boolean_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kBoolean);
-  return absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-      ->boolean_value;
+  return GetProtoValue()->boolean_value;
 }
 
 int64_t FieldValueInternal::integer_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kInteger);
-  return absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-      ->integer_value;
+  return GetProtoValue()->integer_value;
 }
 
 double FieldValueInternal::double_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kDouble);
-  return absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-      ->double_value;
+  return GetProtoValue()->double_value;
 }
 
 Timestamp FieldValueInternal::timestamp_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kTimestamp);
-  const auto& value =
-      absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-          ->timestamp_value;
+  auto& value = GetProtoValue()->timestamp_value;
   return Timestamp{value.seconds, value.nanos};
 }
 
 std::string FieldValueInternal::string_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kString);
-  return MakeString(absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-                        ->string_value);
+  return MakeString(GetProtoValue()->string_value);
 }
 
 const uint8_t* FieldValueInternal::blob_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kBlob);
-  auto value =
-      absl::get<SharedMessage<google_firestore_v1_Value>>(value_)->bytes_value;
+  auto* value = GetProtoValue()->bytes_value;
   return value ? value->bytes : nullptr;
 }
 
 size_t FieldValueInternal::blob_size() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kBlob);
-  auto value =
-      absl::get<SharedMessage<google_firestore_v1_Value>>(value_)->bytes_value;
+  auto* value = GetProtoValue()->bytes_value;
   return value ? value->size : 0;
 }
 
@@ -141,9 +133,7 @@ DocumentReference FieldValueInternal::reference_value() const {
 
 GeoPoint FieldValueInternal::geo_point_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kGeoPoint);
-  const auto& value =
-      absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-          ->geo_point_value;
+  auto& value = GetProtoValue()->geo_point_value;
   return GeoPoint{value.latitude, value.longitude};
 }
 
@@ -164,14 +154,12 @@ std::vector<FieldValue> FieldValueInternal::array_transform_value() const {
 
 std::int64_t FieldValueInternal::integer_increment_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kIncrementInteger);
-  return absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-      ->integer_value;
+  return GetProtoValue()->integer_value;
 }
 
 double FieldValueInternal::double_increment_value() const {
   SIMPLE_HARD_ASSERT(type_ == Type::kIncrementDouble);
-  return absl::get<SharedMessage<google_firestore_v1_Value>>(value_)
-      ->double_value;
+  return GetProtoValue()->double_value;
 }
 
 // Creating sentinels
@@ -304,5 +292,11 @@ std::string Describe(Type type) {
   }
 }
 
+// Helpers
+
+SharedMessage<google_firestore_v1_Value>& GetProtoValue() {
+  return absl::get<SharedMessage<google_firestore_v1_Value>>(value_);
+}
+
 }  // namespace firestore
 }  // namespace firebase

firestore/src/main/field_value_main.h

@@ -73,6 +73,9 @@ class FieldValueInternal {
   explicit FieldValueInternal(FieldValue::Type type, T value)
       : type_{type}, value_{std::move(value)} {}
 
+  /** Returns the underlying value as a google_firestore_v1_Value proto. */
+  nanopb::SharedMessage<google_firestore_v1_Value>& GetProtoValue();
+
   FieldValue::Type type_ = FieldValue::Type::kNull;
   // Note: it's impossible to roundtrip between a `DocumentReference` and
   // `google_firestore_v1_ReferenceValue`, because the latter omits some

firestore/src/main/query_main.cc

@@ -257,7 +257,8 @@ Message<google_firestore_v1_Value> QueryInternal::ConvertDocumentId(
     SimpleThrowInvalidArgument(
         "Invalid query. Expected a string for the document ID.");
   }
-  const std::string& document_id = MakeString(from->string_value);
+
+  std::string document_id = MakeString(from->string_value);
 
   if (!internal_query.IsCollectionGroupQuery() &&
       document_id.find('/') != std::string::npos) {

firestore/src/main/user_data_converter_main.cc

@@ -338,16 +338,16 @@ Message<google_firestore_v1_Value> UserDataConverter::ParseMap(
       const std::string& key = kv.first;
       const FieldValue& value = kv.second;
 
-      auto parsedValue = ParseData(value, context.ChildContext(key));
-      if (parsedValue) {
+      auto parsed_value = ParseData(value, context.ChildContext(key));
+      if (parsed_value) {
         result->map_value.fields[index].key = nanopb::MakeBytesArray(key);
-        result->map_value.fields[index].value = *parsedValue->release();
+        result->map_value.fields[index].value = *parsed_value->release();
         ++index;
       }
     }
   }
 
-  return std::move(result);
+  return result;
 }
 
 void UserDataConverter::ParseSentinel(const FieldValue& value,