Eliminate 'instanceof' checks for FSTFieldValue #2651
Conversation
Will be used instead of isKindOfClass for RTTI to ease migration to c++.
| * The 'type' of this FSTFieldValue. Used for RTTI (rather than isKindOfClass) | ||
| * to ease migration to C++. | ||
| */ | ||
| typedef NS_ENUM(NSInteger, FSTFieldValueType) { |
There was a problem hiding this comment.
Ideally we wouldn't create this, but instead just use model::FieldValue::Type https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/core/src/firebase/firestore/model/field_value.h#L66
There was a problem hiding this comment.
Yeah. I was going to do that conversion as part of step two... but it would be pointless. Fixed.
Previously, we checked for FSTFieldValue subtypes via: `[value isKindOfClass:[FSTStringValue class]]` Now, we check via a new type parameter on FSTFieldValue, i.e.: `[value type] == FSTFieldValueTypeString` This should enable easier transition to C++.
rsgowman
force-pushed
the
rsgowman/fstfv
branch
from
e7da330 to
16b9bec
Compare
| @@ -225,26 +238,24 @@ - (FSTTypeOrder)typeOrder { | |||
| } | |||
|
|
|||
| - (NSComparisonResult)compare:(FSTFieldValue *)other { | |||
| if (![other isKindOfClass:[FSTNumberValue class]]) { | |||
| if (!([other type] == FieldValue::Type::Integer || [other type] == FieldValue::Type::Double)) { | |||
There was a problem hiding this comment.
Maybe extract this into a static bool FieldValue::IsNumber(Type)?
| * Returns the 'type' of this FSTFieldValue. Used for RTTI (rather than isKindOfClass) | ||
| * to ease migration to C++. | ||
| */ | ||
| - (firebase::firestore::model::FieldValue::Type)type; |
There was a problem hiding this comment.
Feel free to insert a using firebase::firestore::model::FieldValue in here to keep usage concise.
Also, FYI if you declared this as
@property(nonatomic, assign, readonly) FieldValue::Type type;And then added @dynamic type; to the @implementation your existing implementation would look like a property that you could access as value.type (without creating a useless ivar).
This isn't going to be around for long so I don't insist at all.
There was a problem hiding this comment.
Done. However, I roughly modelled the 'type' parameter after 'typeOrder', so I assume the same should be done to typeOrder. It turns out, typeOrder was already being used as as value.type, but lacked the @dynamic. So I added that. PTAL to ensure that makes sense.
Previously, we checked for FSTFieldValue subtypes via: `[value isKindOfClass:[FSTStringValue class]]` Now, we check via a new type parameter on FSTFieldValue, i.e.: `value.type == firebase::firestore::model::FieldValue::Type::String` Or with the appropriate using declaration: `value.type == FieldValue::Type::String` This should enable easier transition to C++.
Previously, we checked for FSTFieldValue subtypes via:
[value isKindOfClass:[FSTStringValue class]]Now, we check via a new type parameter on FSTFieldValue, i.e.:
[value type] == FSTFieldValueTypeStringThis should enable easier transition to C++.
(Slightly) more context: go/firestore-cpp-fieldvalue