Skip to content

Commit 6f7ba7a

Browse files
rsgowmanrsgowman
authored
Switch FieldValue to use immutable::sorted_map (instead of std::map) (#2473)
Also avoid static FieldValues since they're not trivially destructible. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
1 parent 286e8f2 commit 6f7ba7a

File tree

10 files changed

+88
-85
lines changed

10 files changed

+88
-85
lines changed

‎Firestore/core/src/firebase/firestore/model/field_value.cc

Lines changed: 39 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -36,23 +36,6 @@ namespace model {
3636
using Type = FieldValue::Type;
3737
using firebase::firestore::util::ComparisonResult;
3838

39-
namespace {
40-
41-
// Makes a copy excluding the specified child, which is expected to be assigned
42-
// different value afterwards.
43-
ObjectValue::Map CopyExcept(const ObjectValue::Map& object_map,
44-
const std::string exclude) {
45-
ObjectValue::Map copy;
46-
for (const auto& kv : object_map) {
47-
if (kv.first != exclude) {
48-
copy[kv.first] = kv.second;
49-
}
50-
}
51-
return copy;
52-
}
53-
54-
} // namespace
55-
5639
FieldValue::FieldValue(const FieldValue& value) {
5740
*this = value;
5841
}
@@ -161,7 +144,7 @@ bool FieldValue::Comparable(Type lhs, Type rhs) {
161144
}
162145

163146
FieldValue FieldValue::Set(const FieldPath& field_path,
164-
FieldValue value) const {
147+
const FieldValue& value) const {
165148
HARD_ASSERT(type() == Type::Object,
166149
"Cannot set field for non-object FieldValue");
167150
HARD_ASSERT(!field_path.empty(),
@@ -170,21 +153,17 @@ FieldValue FieldValue::Set(const FieldPath& field_path,
170153
const std::string& child_name = field_path.first_segment();
171154
const ObjectValue::Map& object_map = object_value_->internal_value;
172155
if (field_path.size() == 1) {
173-
// TODO(zxu): Once immutable type is available, rewrite these.
174-
ObjectValue::Map copy = CopyExcept(object_map, child_name);
175-
copy[child_name] = std::move(value);
176-
return FieldValue::FromMap(std::move(copy));
156+
return SetChild(child_name, value);
177157
} else {
178-
ObjectValue::Map copy = CopyExcept(object_map, child_name);
158+
FieldValue child;
179159
const auto iter = object_map.find(child_name);
180-
if (iter == object_map.end() || iter->second.type() != Type::Object) {
181-
copy[child_name] =
182-
FieldValue::FromMap({}).Set(field_path.PopFirst(), std::move(value));
160+
if (iter != object_map.end() && iter->second.type() == Type::Object) {
161+
child = iter->second;
183162
} else {
184-
copy[child_name] =
185-
iter->second.Set(field_path.PopFirst(), std::move(value));
163+
child = EmptyObject();
186164
}
187-
return FieldValue::FromMap(std::move(copy));
165+
FieldValue new_child = child.Set(field_path.PopFirst(), value);
166+
return SetChild(child_name, new_child);
188167
}
189168
}
190169

@@ -197,21 +176,17 @@ FieldValue FieldValue::Delete(const FieldPath& field_path) const {
197176
const std::string& child_name = field_path.first_segment();
198177
const ObjectValue::Map& object_map = object_value_->internal_value;
199178
if (field_path.size() == 1) {
200-
// TODO(zxu): Once immutable type is available, rewrite these.
201-
ObjectValue::Map copy = CopyExcept(object_map, child_name);
202-
return FieldValue::FromMap(std::move(copy));
179+
return FieldValue::FromMap(object_map.erase(child_name));
203180
} else {
204181
const auto iter = object_map.find(child_name);
205-
if (iter == object_map.end() || iter->second.type() != Type::Object) {
182+
if (iter != object_map.end() && iter->second.type() == Type::Object) {
183+
FieldValue new_child = iter->second.Delete(field_path.PopFirst());
184+
return SetChild(child_name, new_child);
185+
} else {
206186
// If the found value isn't an object, it cannot contain the remaining
207187
// segments of the path. We don't actually change a primitive value to
208188
// an object for a delete.
209189
return *this;
210-
} else {
211-
ObjectValue::Map copy = CopyExcept(object_map, child_name);
212-
copy[child_name] =
213-
object_map.at(child_name).Delete(field_path.PopFirst());
214-
return FieldValue::FromMap(std::move(copy));
215190
}
216191
}
217192
}
@@ -235,28 +210,36 @@ absl::optional<FieldValue> FieldValue::Get(const FieldPath& field_path) const {
235210
return *current;
236211
}
237212

238-
const FieldValue& FieldValue::Null() {
239-
static const FieldValue kNullInstance;
240-
return kNullInstance;
213+
FieldValue FieldValue::SetChild(const std::string& child_name,
214+
const FieldValue& value) const {
215+
HARD_ASSERT(type() == Type::Object,
216+
"Cannot set child for non-object FieldValue");
217+
return FieldValue::FromMap(
218+
object_value_->internal_value.insert(child_name, value));
219+
}
220+
221+
FieldValue FieldValue::Null() {
222+
return FieldValue();
241223
}
242224

243-
const FieldValue& FieldValue::True() {
244-
static const FieldValue kTrueInstance(true);
245-
return kTrueInstance;
225+
FieldValue FieldValue::True() {
226+
return FieldValue(true);
246227
}
247228

248-
const FieldValue& FieldValue::False() {
249-
static const FieldValue kFalseInstance(false);
250-
return kFalseInstance;
229+
FieldValue FieldValue::False() {
230+
return FieldValue(false);
251231
}
252232

253-
const FieldValue& FieldValue::FromBoolean(bool value) {
233+
FieldValue FieldValue::FromBoolean(bool value) {
254234
return value ? True() : False();
255235
}
256236

257-
const FieldValue& FieldValue::Nan() {
258-
static const FieldValue kNanInstance = FieldValue::FromDouble(NAN);
259-
return kNanInstance;
237+
FieldValue FieldValue::Nan() {
238+
return FieldValue::FromDouble(NAN);
239+
}
240+
241+
FieldValue FieldValue::EmptyObject() {
242+
return FieldValue::FromMap(ObjectValue::Empty());
260243
}
261244

262245
FieldValue FieldValue::FromInteger(int64_t value) {
@@ -373,6 +356,11 @@ FieldValue FieldValue::FromMap(ObjectValue::Map&& value) {
373356
return result;
374357
}
375358

359+
bool operator<(const ObjectValue::Map& lhs, const ObjectValue::Map& rhs) {
360+
return std::lexicographical_compare(lhs.begin(), lhs.end(), rhs.begin(),
361+
rhs.end());
362+
}
363+
376364
bool operator<(const FieldValue& lhs, const FieldValue& rhs) {
377365
if (!FieldValue::Comparable(lhs.type(), rhs.type())) {
378366
return lhs.type() < rhs.type();

‎Firestore/core/src/firebase/firestore/model/field_value.h

Lines changed: 34 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,13 @@
1818
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_FIELD_VALUE_H_
1919

2020
#include <cstdint>
21-
#include <map>
2221
#include <memory>
2322
#include <string>
2423
#include <vector>
2524

2625
#include "Firestore/core/include/firebase/firestore/geo_point.h"
2726
#include "Firestore/core/include/firebase/firestore/timestamp.h"
27+
#include "Firestore/core/src/firebase/firestore/immutable/sorted_map.h"
2828
#include "Firestore/core/src/firebase/firestore/model/database_id.h"
2929
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
3030
#include "Firestore/core/src/firebase/firestore/model/field_path.h"
@@ -46,18 +46,7 @@ struct ReferenceValue {
4646
const DatabaseId* database_id;
4747
};
4848

49-
// TODO(rsgowman): Expand this to roughly match the java class
50-
// c.g.f.f.model.value.ObjectValue. Probably move it to a similar namespace as
51-
// well. (FieldValue itself is also in the value package in java.) Also do the
52-
// same with the other FooValue values that FieldValue can return.
53-
class FieldValue;
54-
struct ObjectValue {
55-
// TODO(rsgowman): These will eventually be private. We do want the serializer
56-
// to be able to directly access these (possibly implying 'friend' usage, or a
57-
// getInternalValue() like java has.)
58-
using Map = std::map<std::string, FieldValue>;
59-
Map internal_value;
60-
};
49+
struct ObjectValue;
6150

6251
/**
6352
* tagged-union class representing an immutable data value as stored in
@@ -150,7 +139,7 @@ class FieldValue {
150139
* @param value The value to set.
151140
* @return A new FieldValue with the field set.
152141
*/
153-
FieldValue Set(const FieldPath& field_path, FieldValue value) const;
142+
FieldValue Set(const FieldPath& field_path, const FieldValue& value) const;
154143

155144
/**
156145
* Returns a FieldValue with the field path deleted. If there is no field at
@@ -171,11 +160,12 @@ class FieldValue {
171160
absl::optional<FieldValue> Get(const FieldPath& field_path) const;
172161

173162
/** factory methods. */
174-
static const FieldValue& Null();
175-
static const FieldValue& True();
176-
static const FieldValue& False();
177-
static const FieldValue& Nan();
178-
static const FieldValue& FromBoolean(bool value);
163+
static FieldValue Null();
164+
static FieldValue True();
165+
static FieldValue False();
166+
static FieldValue Nan();
167+
static FieldValue EmptyObject();
168+
static FieldValue FromBoolean(bool value);
179169
static FieldValue FromInteger(int64_t value);
180170
static FieldValue FromDouble(double value);
181171
static FieldValue FromTimestamp(const Timestamp& value);
@@ -193,8 +183,10 @@ class FieldValue {
193183
static FieldValue FromGeoPoint(const GeoPoint& value);
194184
static FieldValue FromArray(const std::vector<FieldValue>& value);
195185
static FieldValue FromArray(std::vector<FieldValue>&& value);
196-
static FieldValue FromMap(const ObjectValue::Map& value);
197-
static FieldValue FromMap(ObjectValue::Map&& value);
186+
static FieldValue FromMap(
187+
const immutable::SortedMap<std::string, FieldValue>& value);
188+
static FieldValue FromMap(
189+
immutable::SortedMap<std::string, FieldValue>&& value);
198190

199191
friend bool operator<(const FieldValue& lhs, const FieldValue& rhs);
200192

@@ -207,6 +199,9 @@ class FieldValue {
207199
*/
208200
void SwitchTo(Type type);
209201

202+
FieldValue SetChild(const std::string& child_name,
203+
const FieldValue& value) const;
204+
210205
Type tag_ = Type::Null;
211206
union {
212207
// There is no null type as tag_ alone is enough for Null FieldValue.
@@ -225,6 +220,24 @@ class FieldValue {
225220
};
226221
};
227222

223+
// TODO(rsgowman): Expand this to roughly match the java class
224+
// c.g.f.f.model.value.ObjectValue. Probably move it to a similar namespace as
225+
// well. (FieldValue itself is also in the value package in java.) Also do the
226+
// same with the other FooValue values that FieldValue can return.
227+
struct ObjectValue {
228+
// TODO(rsgowman): These will eventually be private. We do want the serializer
229+
// to be able to directly access these (possibly implying 'friend' usage, or a
230+
// getInternalValue() like java has.)
231+
using Map = immutable::SortedMap<std::string, FieldValue>;
232+
Map internal_value;
233+
234+
static ObjectValue::Map Empty() {
235+
return Map();
236+
}
237+
};
238+
239+
bool operator<(const ObjectValue::Map& lhs, const ObjectValue::Map& rhs);
240+
228241
/** Compares against another FieldValue. */
229242
bool operator<(const FieldValue& lhs, const FieldValue& rhs);
230243

‎Firestore/core/src/firebase/firestore/model/mutation.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ FieldValue PatchMutation::PatchDocument(const MaybeDocument* maybe_doc) const {
156156
if (maybe_doc && maybe_doc->type() == MaybeDocument::Type::Document) {
157157
return PatchObject(static_cast<const Document*>(maybe_doc)->data());
158158
} else {
159-
return PatchObject(FieldValue::FromMap({}));
159+
return PatchObject(FieldValue::EmptyObject());
160160
}
161161
}
162162

‎Firestore/core/src/firebase/firestore/remote/serializer.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,8 @@ ObjectValue::Map DecodeFields(
139139
const google_firestore_v1_Document_FieldsEntry* fields) {
140140
ObjectValue::Map result;
141141
for (size_t i = 0; i < count; i++) {
142-
result.emplace(DecodeFieldsEntry(reader, fields[i]));
142+
ObjectValue::Map::value_type kv = DecodeFieldsEntry(reader, fields[i]);
143+
result = result.insert(std::move(kv.first), std::move(kv.second));
143144
}
144145

145146
return result;
@@ -173,7 +174,7 @@ ObjectValue::Map DecodeMapValue(Reader* reader,
173174
FieldValue value =
174175
Serializer::DecodeFieldValue(reader, map_value.fields[i].value);
175176

176-
result[key] = value;
177+
result = result.insert(key, value);
177178
}
178179

179180
return result;

‎Firestore/core/test/firebase/firestore/model/document_test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ TEST(Document, Comparison) {
7474

7575
// Document and MaybeDocument will not equal. In particular, Document and
7676
// NoDocument will not equal, which I won't test here.
77-
EXPECT_NE(Document(FieldValue::FromMap({}),
77+
EXPECT_NE(Document(FieldValue(FieldValue::EmptyObject()),
7878
DocumentKey::FromPathString("same/path"),
7979
SnapshotVersion(Timestamp()), DocumentState::kSynced),
8080
UnknownDocument(DocumentKey::FromPathString("same/path"),

‎Firestore/core/test/firebase/firestore/model/field_value_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ TEST(FieldValue, ArrayType) {
189189
}
190190

191191
TEST(FieldValue, ObjectType) {
192-
const FieldValue empty = FieldValue::FromMap({});
192+
const FieldValue empty = FieldValue::FromMap(ObjectValue::Empty());
193193
ObjectValue::Map object{{"null", FieldValue::Null()},
194194
{"true", FieldValue::True()},
195195
{"false", FieldValue::False()}};
@@ -448,7 +448,7 @@ TEST(FieldValue, CompareMixedType) {
448448
const FieldValue geo_point_value = FieldValue::FromGeoPoint({1, 2});
449449
const FieldValue array_value =
450450
FieldValue::FromArray(std::vector<FieldValue>());
451-
const FieldValue object_value = FieldValue::FromMap({});
451+
const FieldValue object_value = FieldValue::EmptyObject();
452452
EXPECT_TRUE(null_value < true_value);
453453
EXPECT_TRUE(true_value < number_value);
454454
EXPECT_TRUE(number_value < timestamp_value);

‎Firestore/core/test/firebase/firestore/model/mutation_test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ TEST(Mutation, DeletesValuesFromTheFieldMask) {
114114
{"baz", FieldValue::FromString("baz-value")}})}}));
115115

116116
std::unique_ptr<Mutation> patch =
117-
PatchMutation("collection/key", {}, {Field("foo.bar")});
117+
PatchMutation("collection/key", ObjectValue::Empty(), {Field("foo.bar")});
118118

119119
MaybeDocumentPtr patch_doc =
120120
patch->ApplyToLocalView(base_doc, base_doc.get(), Timestamp::Now());

‎Firestore/core/test/firebase/firestore/remote/serializer_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,7 @@ TEST_F(SerializerTest, EncodesTimestamps) {
465465
}
466466

467467
TEST_F(SerializerTest, EncodesEmptyMap) {
468-
FieldValue model = FieldValue::FromMap({});
468+
FieldValue model = FieldValue::EmptyObject();
469469

470470
v1::Value proto;
471471
proto.mutable_map_value();
@@ -797,7 +797,7 @@ TEST_F(SerializerTest, BadKey) {
797797

798798
TEST_F(SerializerTest, EncodesEmptyDocument) {
799799
DocumentKey key = DocumentKey::FromPathString("path/to/the/doc");
800-
FieldValue empty_value = FieldValue::FromMap({});
800+
FieldValue empty_value = FieldValue::EmptyObject();
801801
SnapshotVersion update_time = SnapshotVersion{{1234, 5678}};
802802

803803
v1::BatchGetDocumentsResponse proto;

‎Firestore/core/test/firebase/firestore/testutil/testutil.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ std::unique_ptr<model::PatchMutation> PatchMutation(
2727
const model::ObjectValue::Map& values,
2828
// TODO(rsgowman): Investigate changing update_mask to a set.
2929
const std::vector<model::FieldPath>* update_mask) {
30-
model::FieldValue object_value = model::FieldValue::FromMap({});
30+
model::FieldValue object_value = model::FieldValue::EmptyObject();
3131
std::set<model::FieldPath> object_mask;
3232

3333
for (const auto& kv : values) {

‎Firestore/core/test/firebase/firestore/testutil/testutil.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ inline model::SnapshotVersion Version(int64_t version) {
8080
inline model::Document Doc(
8181
absl::string_view key,
8282
int64_t version = 0,
83-
const model::ObjectValue::Map& data = {},
83+
const model::ObjectValue::Map& data = model::ObjectValue::Empty(),
8484
model::DocumentState document_state = model::DocumentState::kSynced) {
8585
return model::Document{model::FieldValue::FromMap(data), Key(key),
8686
Version(version), document_state};
@@ -140,15 +140,16 @@ inline core::Query Query(absl::string_view path) {
140140
}
141141

142142
inline std::unique_ptr<model::SetMutation> SetMutation(
143-
absl::string_view path, const model::ObjectValue::Map& values = {}) {
143+
absl::string_view path,
144+
const model::ObjectValue::Map& values = model::ObjectValue::Empty()) {
144145
return absl::make_unique<model::SetMutation>(
145146
Key(path), model::FieldValue::FromMap(values),
146147
model::Precondition::None());
147148
}
148149

149150
std::unique_ptr<model::PatchMutation> PatchMutation(
150151
absl::string_view path,
151-
const model::ObjectValue::Map& values = {},
152+
const model::ObjectValue::Map& values = model::ObjectValue::Empty(),
152153
const std::vector<model::FieldPath>* update_mask = nullptr);
153154

154155
inline std::unique_ptr<model::PatchMutation> PatchMutation(

0 commit comments

Comments
 (0)