implement FieldValue for null, boolean, and array in C++.

[zxu123]

Discussion

go/firestorecppfieldvalue

Testing

unit test

API Changes

n/a

[wilhuff]

This is firebase_firestore_model :-).

There shouldn't be any multi-platform shenanigans here so I think you can just remove this comment.

[zxu123]

Done. It's a bad copy&paste =D

[wilhuff]

newline between the include of this class and anything else.

[zxu123]

Done

[wilhuff]

Prefer enum class TypeOrder { ... }; to this kind of enum. The values inside the enumeration can be unqualified (i.e. they can be Null, Boolean, etc.).

[zxu123]

Done

[wilhuff]

Similar feedback here: enum class Type. Also Type is the principal concept here and TypeOrder is secondary.

You may even be able to make TypeOrder a private enum.

[zxu123]

Done. TypeOrder is used in subtype and test; so not making private.

[wilhuff]

As discussed offline, let's see if we can make this a non-virtual setup using a tagged union. With this arrangement all FieldValues will need to be allocated on the heap and accessed by pointer.

If the tagged union mechanism doesn't work then this class needs a virtual destructor.

[zxu123]

Done. Just one thing. Now since each subtype has no relation with FieldValue, the old logic of constant value (NullValue, TrueValue, FalseValue) needs to be defined in a reasonable way. I am thinking make constants of type FieldValue (instead of each individual subtype) but not sure. Generally those subtype is kinda internal to the FieldValue and user should almost use FieldValue. Then one can assign a FieldValue to one of those constant value to save a bit footprint on memory and cpu for allocation. How do you think?

[wilhuff]

I think the extra types (NullValue, BooleanValue, ArrayValue, etc) don't really buy us anything.

We can just make the values of the underlying types direct members of the union.

At that point, yes, I agree the static instances should be of type FieldValue :-).

[zxu123]

done

[wilhuff]

The C++ idiom for comparison is operator<.

Probably this should declared as

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

[zxu123]

Compare returns three values, -1, 0, 1. We can add the common comparison operator on top of Compare(); and probably make Compare() private.

[wilhuff]

compare: is an Objective-C mechanism, defined for compatibility with Objective-C containers. You can see how native types implement it:

https://developer.apple.com/documentation/foundation/nsnumber/1413562-compare?language=objc

In C++ ordered containers use a comparator whose default is usually std::less which invokes operator< on the types to be inserted. Because of this no external usage of Compare should ever exist.

The standard way to implement the other operators is like this:

inline bool operator> (const X& lhs, const X& rhs){ return rhs < lhs; }
inline bool operator<=(const X& lhs, const X& rhs){ return !(lhs > rhs); }
inline bool operator>=(const X& lhs, const X& rhs){ return !(lhs < rhs); }

Meanwhile for any type that implements operator< the multi-valued return can be implemented generically (though I'm probably getting the direction wrong). Indeed we can include something like this in our toolbox for interoperating with Objective-C.

template <typename T>
NSComparisonResult Compare(const T& lhs, const T& rhs) {
  if (lhs < rhs) {
    return NSOrderedAscending;
  } else if (rhs < lhs) {
    return NSOrderedDescending;
  } else {
    return NSOrderedSame;
  }

[wilhuff]

I think the extra types (NullValue, BooleanValue, ArrayValue, etc) don't really buy us anything.

We can just make the values of the underlying types direct members of the union.

At that point, yes, I agree the static instances should be of type FieldValue :-).

[wilhuff]

compare: is an Objective-C mechanism, defined for compatibility with Objective-C containers. You can see how native types implement it:

https://developer.apple.com/documentation/foundation/nsnumber/1413562-compare?language=objc

In C++ ordered containers use a comparator whose default is usually std::less which invokes operator< on the types to be inserted. Because of this no external usage of Compare should ever exist.

The standard way to implement the other operators is like this:

inline bool operator> (const X& lhs, const X& rhs){ return rhs < lhs; }
inline bool operator<=(const X& lhs, const X& rhs){ return !(lhs > rhs); }
inline bool operator>=(const X& lhs, const X& rhs){ return !(lhs < rhs); }

Meanwhile for any type that implements operator< the multi-valued return can be implemented generically (though I'm probably getting the direction wrong). Indeed we can include something like this in our toolbox for interoperating with Objective-C.

template <typename T>
NSComparisonResult Compare(const T& lhs, const T& rhs) {
  if (lhs < rhs) {
    return NSOrderedAscending;
  } else if (rhs < lhs) {
    return NSOrderedDescending;
  } else {
    return NSOrderedSame;
  }

[wilhuff]

If both types are Null they compare equal by definition so there's nothing to actually compare (i.e. this can always just return 0;).

[zxu123]

done

[wilhuff]

You can cut down on the verbosity in here by doing this after the #includes

using FieldValue::Type;
using FieldValue::TypeOrder;

[zxu123]

Done

[wilhuff]

It's worth noting in this comment why this exists separately from Type.

In particular: Long and Double are compared together and Timestamp and ServerTimestamp are compared together.

I wonder if we can get rid of TypeOrder altogether if we have a Comparable(Type left_type, Type right_type) function?

[zxu123]

I like that idea of getting rid of TypeOrder. We can also just switch-case on the implied TypeOrder and do comparison without any other helper function. The only thing to take care of is to group the Type of the same TypeOrder in the enum definition in the same TypeOrder order as server does. Since we do not expose Type to public api, we are free to rearrange the enum order.

[wilhuff]

The type is Blob, not Binary.

[zxu123]

Done

[wilhuff]

Instead of creating separate types for these I think you could just have static factory methods on FieldValue. Something like

class FieldValue {
 public:
  static const FieldValue& NullValue();
  static const FieldValue& BooleanValue(bool value);
  // ...
}

const FieldValue& FieldValue::NullValue() {
  static FieldValue kNullInstance(Type::Null);
  return kNullInstance;
}

const FieldValue& FieldValue::BooleanValue(bool value) {
  static FieldValue kTrueInstance(Type::Boolean, true);
  static FieldValue kFalseInstance(Type::Boolean, false);
  return value ? kTrueInstance : kFalseInstance;
}

[zxu123]

done

[zxu123]

refactored. PTAL.

[zxu123]

Done

[zxu123]

done

[zxu123]

Done

[zxu123]

I like that idea of getting rid of TypeOrder. We can also just switch-case on the implied TypeOrder and do comparison without any other helper function. The only thing to take care of is to group the Type of the same TypeOrder in the enum definition in the same TypeOrder order as server does. Since we do not expose Type to public api, we are free to rearrange the enum order.

[zxu123]

done

[zxu123]

done

[wilhuff]

Despite all the comments in here I really like the direction this is heading.

In particular, the insight that we could use placement new to initialize the union is fantastic. It's bulletproof and doesn't require memset or any other hacks like that.

[wilhuff]

Since you're not spelling the unqualified type name differently this can just be

using FieldValue::Type;

[zxu123]

Does not work since FieldValue is not namespace.

/Users/zxu/Documents/GitHub/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/field_value.cc:29:19: error: using declaration
cannot refer to class member
using FieldValue::Type;
~~~~~~~~~~~~^
/Users/zxu/Documents/GitHub/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/field_value.cc:29:7: note: use an alias
declaration instead
using FieldValue::Type;
^
Type =

[wilhuff]

Surprising! Thanks for pointing out the details.

[wilhuff]

Please add comments. In particular since this deviates from the other platforms it's worth commenting on how this is different and why.

In this case, this is equivalent to the type order on the other platforms.

[zxu123]

done

[wilhuff]

Nice.

[wilhuff]

./scripts/style.sh should align these comments.

[zxu123]

done

[zxu123]

done

[wilhuff]

The order is defined publicly. Perhaps link to this document?

https://firebase.google.com/docs/firestore/manage-data/data-types

[zxu123]

done

[zxu123]

done

[wilhuff]

Comments please. In particular it's useful to know that after it runs, the type will be set to Null.

[zxu123]

After thinking it twice, I think the combine of the two helper is more than enough a.k.a. we can reset by copy from null. The extra cost is minimal. In other words, all the logic can be put in operator= directly.

[zxu123]

ack.

[wilhuff]

Sorry I gave you the wrong idea here. This should be return false; since the type is bool.

[zxu123]

done

[wilhuff]

Missing newline.

[zxu123]

done

[wilhuff]

Please add a test of assignment. In particular, please add a test showing something like this works and verify that we correctly destroy the vector.

FieldValue value = FieldValue::NullValue();
value = FieldValue::ArrayValue(std::vector<const FieldValue>{
  FieldValue::BooleanValue(true),
  FieldValue::BooleanValue(false)
});
value = FieldValue::NullValue();

[zxu123]

done

[wilhuff]

The javascript SDK defines BooleanValue::TRUE.

For consistency's sake we should probably have

static const FieldValue& True();
static const FieldValue& False();

in addition to this method. As in the JS SDK, this implementation can delegate to those as appropriate.

[zxu123]

Done. We already have NullValue(); so added TrueValue() and FalseValue(). Let me know if you prefer True() and False() (and Null()).

[zxu123]

refactored and lint-fixed.

[zxu123]

Does not work since FieldValue is not namespace.

/Users/zxu/Documents/GitHub/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/field_value.cc:29:19: error: using declaration
cannot refer to class member
using FieldValue::Type;
~~~~~~~~~~~~^
/Users/zxu/Documents/GitHub/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/field_value.cc:29:7: note: use an alias
declaration instead
using FieldValue::Type;
^
Type =

[zxu123]

done

[zxu123]

Yes, at least for cmake:

/Users/zxu/Documents/GitHub/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/field_value.cc:60:13: error:
label at end of compound statement: expected statement
default:
^
;
1 error generated.

[zxu123]

done

[zxu123]

done

[zxu123]

done

[zxu123]

done

[zxu123]

done

[zxu123]

done

[zxu123]

done

[wilhuff]

This PR is now very close. Thanks for working through this.

[wilhuff]

Surprising! Thanks for pointing out the details.

[wilhuff]

Isn't the std::move unnecessary here since the anonymous std::vector you've constructed here is already an rvalue?

This would be a little more obvious if you split the copy up from the invocation of the move constructor, e.g.

std::vector<const FieldValue> copy(value);
return ArrayValue(std::move(copy));

However, if you're just going to immediately copy the incoming value you can declare the parameter as by-value and let the compiler do it for you.

I.e. you can make this:

FieldValue FieldValue::ArrayValue(std::vector<const FieldValue> value) {
  return ArrayValue(std::move(value));
}

[wilhuff]

Compiled under Xcode this also generates a warning:

⚠️ /Users/travis/build/firebase/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/field_value.cc:119:21: moving a temporary object prevents copy elision [-Wpessimizing-move]
return ArrayValue(std::move(std::vector(value)));
^

You could definitely fix this just by removing the std::move.

[zxu123]

Done, going with the first way. The second way needs to remove the other constructor by rvalue reference or get ambiguous definition. So I'd keep both constructor, copy and move, which is now common in C++11 e.g. in std::vector (http://www.cplusplus.com/reference/vector/vector/vector/).

[wilhuff]

This is also copy. Did you mean to make this a reference? (const FieldValue& null_value would do it).

[zxu123]

Sorry, bad name. Meant to test copy operator=. (The next test case tests move operator=.)

[wilhuff]

Also: FieldValue::TrueValue() should remain unchanged:

EXPECT_EQUAL(Type::Boolean, FieldValue::TrueValue());

Or something like that.

[zxu123]

Done

[wilhuff]

I'm really surprised this works.

The point of std::move to indicate that after the call that name will not be used again. The value with that name becomes invalid immediately after std::move returns (but before the assignment).

See here for more discussion:
https://stackoverflow.com/questions/24604285/behaviour-of-move-assignment-to-self

Sorry, I should have been clearer: it's the copy constructor/copy assignment that need to work when copying *this.

[zxu123]

This works since the move operator= implementation for FieldValue uses std::swap and thus both sides are still well-defined after the call. I can go the other design (self-check+move), if that is more preferred. How do you think?

[wilhuff]

I don't understand what the names small_value_array and large_value_array mean here. The small one seems longer than the large one.

Also, you can skip the assignment when initializing.

std::vector<const FieldValue> small_value_array{
  // ...
};

[zxu123]

Done. small and large means the FieldValue constructed from them are small and large respective, not the array itself. Let me change it to another set of name to avoid confusion.

[wilhuff]

This is a surprising whitespace change. Are you on clang-format 6?

[zxu123]

Yes, clang-format version 6.0.0 (tags/google/stable/2017-11-14).

[wilhuff]

Something is wrong with our assertion declaration because this is generating a compile-time error:

❌ /Users/travis/build/firebase/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/field_value.cc:146:1: control may reach end of non-void function [-Werror,-Wreturn-type]

[zxu123]

I guess it is possible assertion does not break the flow for certain build (this case here) and thus still require a return value. The C++ sdk actually have macros like FIREBASE_ASSERT_RETURN and FIREBASE_ASSERT_RETURN_VOID, which however does not support message. Let me add a return value explicitly.

[zxu123]

Please take another look

[wilhuff]

LGTM