Implement the rest of FieldValue types for C++

[zxu123]

Discussion

go/firestorecppfieldvalue

Testing

Unit test

API Changes

go/firestorecppfieldvalue

[wilhuff]

I've made a first pass on this but let's discuss before this goes too much farther.

[wilhuff]

Property accessors should be lower-case.

[zxu123]

ack

[wilhuff]

Please define swap as a friend function suitable for use with ADL if we can.

This should be

friend void swap(Blob& lhs, Blob& rhs);

You should be able to call swap like this:

TEST(Blob, SwapsViaArgumentDependentLookup) {
  Blob foo = Blob.CopyFrom("foo", 4);
  Blob bar = Blob.CopyFrom("bar", 4);
  
  using std::swap;
  swap(left, right);
 
  EXPECT_EQ("bar", static_cast<const char*>(foo.get()));
  EXPECT_EQ("foo", static_cast<const char*>(bar.get()));
}

[zxu123]

ack. Don't need it anymore since we now use C++11 language feature and have rvalue overload of operator=.

[wilhuff]

Note that while we cannot use C++11 library features because of STLport, all target platforms at least support C++11 language features.

Our worst-case platform for this API is NDK 10d using the STLport runtime which has Clang 3.5 and GCC 4.6 as compilers.. Clang 3.5 implements C++14 and GCC 4.6 implements enough of C++11 that we can declare move constructors.

[zxu123]

ack

[wilhuff]

Please declare this publicly as a deleted constructor:

Blob() = delete;

[zxu123]

Still need a private one to initialize a Blob inside builder function.

[wilhuff]

This is a problematic API:

1. In the implementation free() on this pointer, but you haven't specified that the source should have been allocated with malloc(). The value could just have been allocated with new[] which will cause a mismatch and corrupt memory: https://blogs.msdn.microsoft.com/oldnewthing/20040203-00/?p=40763/#66782

2. To remedy this, we might allow the user to supply a deleter function for us to call back when we're done with the value, but this is problematic because:

   • This value is supposed to be scalar-like
   • so it won't be be connected to any particular instance of Firestore
   • so we won't have access to a user callback queues in this class's destructor
   • which means we'd have to execute the deleter in the current thread.
   • We do not allow user code to run in a Firestore worker thread
   • so we just can't accept values with a user deleter into the core.
   • In turns this means we'd have to copy the value which defeats the point of this API

3. The RTDB variant has a different contract: either the value is copied in (known there as mutable) or not copied/not freed (known there as static). This is free of this API problem but requires us to know whether or not to delete the blob.

[zxu123]

now the Blob is internal and we assume it wraps byte arrays. It also owns the wrapped array, either a copy of user's data or the moved user's data, depending on which builder function to call. In other words, our Blob is always in the mutable sense and never in static sense in the RTDB point of view.

[wilhuff]

Timestamps in the existing iOS implementation don't have a notion of an "origin" timestamp and don't treat timestamps specially based on them being origins or not. In particular I'm not sure this is even a good idea because users are allowed to use such a value for their own purposes so we can't make origins special.

[zxu123]

The problem here in C++ is that iOS previous_value is Nullable and thus we need a dummy value for previous_value to present the null value. How about make a null value of timestamp that is not overlap with any true timestamp and just call the API function bool Timestamp::IsNull() and static Timestamp Timestamp::NullValue()?

[wilhuff]

Let's reconcile this with @varconst-fb since this will be a public API.

[varconst-fb]

@zxu123, do you refer to previousValue in FSTServerTimestampValue? If yes, perhaps you could store previousValue as absl::optional, and for constructor/factory function, provide two overloads, with and without previousValue?

[zxu123]

I'll port abseil's optional in a separate PR. @wilhuff any comment on porting abseil's optional? The port only introduce additional:

• absl/memory/memory.{h,cc},
• absl/base/macros.{},
• absl/base/port.h,
• absl/base/attributes.,
• abs/base/config.,
• abs/base/optimization.,
• abs/utility/utility.,
• absl/base/internal/invoke.h,
• absl/types/bad_optional_access.h,

absl::optional might be also useful for other part of the C++ code due to the lack of mechanism of C++ to allow value types being null.

[var-const]

I presumed we depended on Abseil, so I didn't expect this to be an issue. I guess it's possible to just have a flag inside ServerTimestamp like bool has_previous_value_. Unless there are other places which need some sort of a NaN timestamp, I'd keep this quirk inside implementation of ServerTimestamp instead of introducing a sentinel Timestamp value.

OTOH, optional will probably find other uses. I'll defer to @wilhuff on this.

[wilhuff]

Based on these comments yesterday I started work on just doing a full import of abseil rather than doing it piecewise as we've done so far. I've come across a blocking issue I hope to work out with the abseil team today (though who knows how long it will take for my proposal to actually land in their github repo).

For now I would add the has_previous_value_ and a TODO so we can make progress without that dependency.

[wilhuff]

Please preserve the more descriptive names we had before. local_write_time conveys more than just local.

[zxu123]

done

[wilhuff]

The timestamp constructor would throw on these arguments.

Perhaps we're better off just making Timestamp default constructable.

[zxu123]

There is already a constructor of Timestamp with no parameter, which sets timestamp to now. To get now is costly in particular what the value here does not really matter and thus that operation is in waste.

0 seconds and 0 nanos is a valid timestamp by design, see the assertion in iOS implementation:

    FSTAssert(nanos >= 0, @"timestamp nanoseconds out of range: %d", nanos);
    FSTAssert(nanos < 1e9, @"timestamp nanoseconds out of range: %d", nanos);
    // Midnight at the beginning of 1/1/1 is the earliest timestamp Firestore supports.
    FSTAssert(seconds >= -62135596800L, @"timestamp seconds out of range: %lld", seconds);
    // This will break in the year 10,000.
    FSTAssert(seconds < 253402300800L, @"timestamp seconds out of range: %lld", seconds);

The alternative design (and divert from the other SDK's) is to allow default construction to set everything to 0 and define a builder method to return timestamp that contains now.

[wilhuff]

Okay, Timestamp is going to be a public type so we need to take some care.

My intuition is that Timestamp should default construct with zero values and we should add a Timestamp::Now() factory function to actually get current time.

/cc @varconst-fb

[varconst-fb]

I'm in favor of having a default constructor that initializes the value to zero:

• Timestamp looks like it should have value semantics;
• I think it's more idiomatic for a default-constructed object to be in an "empty" state;
• it's consistent with what std::chrono::time_point does.

[zxu123]

SGTM. I think we may want a document to record the diversion of C++ implementation and ObjC implementation; here the default ctor behavior of Timestamp is one. This way may help people avoid making certain type of mistake like replace the construct of Timestamp but forgot to initialize with Now.

[wilhuff]

We've done this in the past with "PORTING NOTE" in the comments of the class in question where there's a divergence.

https://github.com/firebase/firebase-js-sdk/search?utf8=%E2%9C%93&q=%22PORTING+NOTE%22&type=

[zxu123]

done

[wilhuff]

If Timestamp were default constructable then you wouldn't need to call this constructor explicitly.

[zxu123]

ack. see last comment.

[zxu123]

fixed

[wilhuff]

Types that aren't default-constructable aren't very user friendly if they're intended to be used as value types.

We should just make these have a default constructor so that we're not littering the callers with this kind of stuff.

[zxu123]

None of the other client SDK's has constructor of GeoPoint without parameters. On the other hand, the C++ language does make it different from other SDK, where variables are mostly reference. Let me add default constructor to GeoPoint for now; probably worth to discuss since this goes into the public api geo_point.h.

[zxu123]

PTAL

[zxu123]

That's true. Let me the Blob definition into /model/; this comes handy to define FieldValue union since blob needs both allocation and size.

[zxu123]

ack

[zxu123]

now the Blob is internal and we assume it wraps byte arrays. It also owns the wrapped array, either a copy of user's data or the moved user's data, depending on which builder function to call. In other words, our Blob is always in the mutable sense and never in static sense in the RTDB point of view.

[zxu123]

ack

[zxu123]

ack. Don't need it anymore since we now use C++11 language feature and have rvalue overload of operator=.

[zxu123]

There is already a constructor of Timestamp with no parameter, which sets timestamp to now. To get now is costly in particular what the value here does not really matter and thus that operation is in waste.

0 seconds and 0 nanos is a valid timestamp by design, see the assertion in iOS implementation:

    FSTAssert(nanos >= 0, @"timestamp nanoseconds out of range: %d", nanos);
    FSTAssert(nanos < 1e9, @"timestamp nanoseconds out of range: %d", nanos);
    // Midnight at the beginning of 1/1/1 is the earliest timestamp Firestore supports.
    FSTAssert(seconds >= -62135596800L, @"timestamp seconds out of range: %lld", seconds);
    // This will break in the year 10,000.
    FSTAssert(seconds < 253402300800L, @"timestamp seconds out of range: %lld", seconds);

The alternative design (and divert from the other SDK's) is to allow default construction to set everything to 0 and define a builder method to return timestamp that contains now.

[zxu123]

ack. see last comment.

[zxu123]

None of the other client SDK's has constructor of GeoPoint without parameters. On the other hand, the C++ language does make it different from other SDK, where variables are mostly reference. Let me add default constructor to GeoPoint for now; probably worth to discuss since this goes into the public api geo_point.h.

[zxu123]

done

[zxu123]

The problem here in C++ is that iOS previous_value is Nullable and thus we need a dummy value for previous_value to present the null value. How about make a null value of timestamp that is not overlap with any true timestamp and just call the API function bool Timestamp::IsNull() and static Timestamp Timestamp::NullValue()?

[wilhuff]

More feedback to keep you going.

[wilhuff]

Why is this required?

[zxu123]

fixed

[wilhuff]

If we intend GeoPoint to be copy/move constructable using the generated constructors we should document it:

GeoPoint(const GeoPoint& other) = default;
GeoPoint(GeoPoint&& other) = default;

This has the extra benefit that should we make a change that breaks the compilers ability to generate these constructors it will fail our build rather than just silently omitting them and breaking our customers' builds.

[zxu123]

Done. Explicitly makes those default; frankly speaking, I don't aware any compiler that will omit those default in our case here.

  GeoPoint(const GeoPoint& other) = default;
  GeoPoint(GeoPoint&& other) = default;
  GeoPoint& operator=(const GeoPoint& other) = default;
  GeoPoint& operator=(GeoPoint&& other) = default;

[wilhuff]

Sorry I should have been clearer in what I meant.

I agree that no compiler would omit these today. However, should we accidentally make a change to this class that made it not trivially copyable then we might not realize it before we broke our customers. If you include these declarations and the compiler cannot generate the default implementations because we've made such a change it will fail the build.

Since this is a public API I'm advocating that it's worth it to protect us from ourselves :-).

[wilhuff]

Please port documentation comments from the objective-C implementation:

https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Public/FIRGeoPoint.h

[zxu123]

done

[wilhuff]

It doesn't look like any of these headers are used (besides firebase_assert)

[zxu123]

isnan in math.h. The other std library is not used and removed.

[wilhuff]

Ah, sorry missed that.

[wilhuff]

This is std::lexicographical_compare. Use it like so:

return std::lexicographical_compare(
    lhs.begin(), lhs.end(),
    rhs.begin(), rhs.end());

This presupposes that blob has begin() and end() functions that make it compatible with normal C++ container-like utilities like so:

const uint8_t* begin() const {
  return buffer_;
}

const uint8_t* end() const {
  return buffer_ + size_;
}

However, see above: if make the buffer a std::vector then you can just use the comaparator I've already defined:

firebase-ios-sdk/Firestore/core/src/firebase/firestore/util/comparison.h

Line 110 in d764ad2

struct Comparator<std::vector<uint8_t>>

[zxu123]

done

[wilhuff]

Since we're always copying now, is there any reason not to make our internal buffer just use std::vector<uint8_t>?

At that point we stop having to worry about managing pointers at all.

Given an external source and size, you can construct the vector directly:

std::vector<uint8_t> buffer(source, source + size);

[zxu123]

It is not always copying; it always takes ownership a.k.a. either make a copy and take ownership of that copy (by calling CopyFrom) or directly take the passing-in's ownership and not make a copy (by calling MoveFrom). There is no way to achieve the MoveFrom(const uint8_t* source, size_t size) via std::vector.

[wilhuff]

What you're saying is true (that we can't move from something else while using vector) but the reality is that the the public API is going to be firebase::Variant which has two kinds of values:

• "mutable" blobs, whose values were allocated by the Variant
• "static" blobs, whose values were passed by the caller and the caller still owns

You can't move from either of these:

• In the "mutable" case the user has declared the variant to have a type of value and we can't change that type. Therefore the buffer is owned by the Variant.
• In the "static" case we can't take control of the user buffer.

[zxu123]

I think the discussion went too far on the Blob for implementing FieldValue type. Since CPU cost is not a main concern as in our OKR discussion for now while code size and complexity is the main concern, let me just use the std::vector<uint8_t> in FieldValue. There is no point to wrap around a std::vector<uint8_t> via a Blob here as all the Blob API in the end is simply dedicated to std::vector API calls.

[wilhuff]

This seems like a reasonable conclusion.

[wilhuff]

This seems suboptimal because it requires two memcmps. Better to define equality directly. If you make buffer_ a vector then this can just be lhs.buffer_ == rhs.buffer_.

[zxu123]

ack. reimplemented. cannot use vector, see comment above.

[wilhuff]

Okay, Timestamp is going to be a public type so we need to take some care.

My intuition is that Timestamp should default construct with zero values and we should add a Timestamp::Now() factory function to actually get current time.

/cc @varconst-fb

[wilhuff]

Let's reconcile this with @varconst-fb since this will be a public API.

[zxu123]

Updated. Should I port absl::optional? If so, I'll do it in a separate PR and then address the change here.

[zxu123]

fixed

[zxu123]

Done. Explicitly makes those default; frankly speaking, I don't aware any compiler that will omit those default in our case here.

  GeoPoint(const GeoPoint& other) = default;
  GeoPoint(GeoPoint&& other) = default;
  GeoPoint& operator=(const GeoPoint& other) = default;
  GeoPoint& operator=(GeoPoint&& other) = default;

[zxu123]

done

[zxu123]

isnan in math.h. The other std library is not used and removed.

[zxu123]

It is not always copying; it always takes ownership a.k.a. either make a copy and take ownership of that copy (by calling CopyFrom) or directly take the passing-in's ownership and not make a copy (by calling MoveFrom). There is no way to achieve the MoveFrom(const uint8_t* source, size_t size) via std::vector.

[zxu123]

done

[zxu123]

ack. reimplemented. cannot use vector, see comment above.

[zxu123]

SGTM. I think we may want a document to record the diversion of C++ implementation and ObjC implementation; here the default ctor behavior of Timestamp is one. This way may help people avoid making certain type of mistake like replace the construct of Timestamp but forgot to initialize with Now.

[zxu123]

fixed

[zxu123]

I'll port abseil's optional in a separate PR. @wilhuff any comment on porting abseil's optional? The port only introduce additional:

• absl/memory/memory.{h,cc},
• absl/base/macros.{},
• absl/base/port.h,
• absl/base/attributes.,
• abs/base/config.,
• abs/base/optimization.,
• abs/utility/utility.,
• absl/base/internal/invoke.h,
• absl/types/bad_optional_access.h,

absl::optional might be also useful for other part of the C++ code due to the lack of mechanism of C++ to allow value types being null.

[wilhuff]

Ah, sorry missed that.

[wilhuff]

What you're saying is true (that we can't move from something else while using vector) but the reality is that the the public API is going to be firebase::Variant which has two kinds of values:

• "mutable" blobs, whose values were allocated by the Variant
• "static" blobs, whose values were passed by the caller and the caller still owns

You can't move from either of these:

• In the "mutable" case the user has declared the variant to have a type of value and we can't change that type. Therefore the buffer is owned by the Variant.
• In the "static" case we can't take control of the user buffer.

[wilhuff]

We've done this in the past with "PORTING NOTE" in the comments of the class in question where there's a divergence.

https://github.com/firebase/firebase-js-sdk/search?utf8=%E2%9C%93&q=%22PORTING+NOTE%22&type=

[wilhuff]

nit: It would help the readability of these tests if you made little functions to hide these casts.

Something like

const uint8_t* Bytes(const char* value) { ... }
uint8_t* Bytes(char* value) { ... }

And perhaps the other direction too.

[zxu123]

done

[wilhuff]

Use EXPECT_STREQ() instead of EXPECT_EQ(0, strcmp())

[zxu123]

ack

[wilhuff]

EXPECT_LT?

[zxu123]

Not sure. Here we want explicitly show that we are testing the operator<, not depending on the assumption EXPECT_LT translates to operator<.

[wilhuff]

I think that's a safe assumption to make. Also it will give better error messages, but it's not that big of deal one way or the other.

[zxu123]

Updated. PTAL.

[zxu123]

I think the discussion went too far on the Blob for implementing FieldValue type. Since CPU cost is not a main concern as in our OKR discussion for now while code size and complexity is the main concern, let me just use the std::vector<uint8_t> in FieldValue. There is no point to wrap around a std::vector<uint8_t> via a Blob here as all the Blob API in the end is simply dedicated to std::vector API calls.

[zxu123]

done

[zxu123]

done

[zxu123]

ack

[zxu123]

Not sure. Here we want explicitly show that we are testing the operator<, not depending on the assumption EXPECT_LT translates to operator<.

[wilhuff]

LGTM, though we should start to think about porting some of the test helpers to C++ so that we can cut down on the verbosity of instantiating object values and the like.

I'd suggest that we can probably follow the Android port more closely, though we need to decide if we want the helpers to remain free functions or what.

[wilhuff]

Sorry I should have been clearer in what I meant.

I agree that no compiler would omit these today. However, should we accidentally make a change to this class that made it not trivially copyable then we might not realize it before we broke our customers. If you include these declarations and the compiler cannot generate the default implementations because we've made such a change it will fail the build.

Since this is a public API I'm advocating that it's worth it to protect us from ourselves :-).

[wilhuff]

This seems like a reasonable conclusion.

[wilhuff]

I think that's a safe assumption to make. Also it will give better error messages, but it's not that big of deal one way or the other.