Skip to content

Commit

Permalink
Port comparison to C++ (#678)
Browse files Browse the repository at this point in the history
This reimplements our comparison functions as C++ Comparators and then provides compatibility shims for interoperating with existing Objective-C usage.

A few specialized comparators aren't suitable for porting but only have a single usage (e.g. CompareBytes for comparing NSData * instances). In these cases I've moved them into the caller.

* Use int32_t for typeof(ID) in FSTDocumentReference

* Migrate callers of FSTComparison.h to Objective-C++

* Port comparison to C++

* Migrate usages of FSTComparison.h to C++ equivalents

* Remove FSTComparison
  • Loading branch information
wilhuff authored Jan 19, 2018
1 parent 9f7c094 commit 39d8252
Show file tree
Hide file tree
Showing 17 changed files with 614 additions and 420 deletions.
8 changes: 4 additions & 4 deletions Firestore/Example/Firestore.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
54764FAB1FAA0C320085E60A /* string_util_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54764FAA1FAA0C320085E60A /* string_util_test.cc */; };
54764FAF1FAA21B90085E60A /* FSTGoogleTestTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 54764FAE1FAA21B90085E60A /* FSTGoogleTestTests.mm */; };
548DB927200D590300E00ABC /* assert_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 548DB926200D590300E00ABC /* assert_test.cc */; };
548DB929200D59F600E00ABC /* comparison_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 548DB928200D59F600E00ABC /* comparison_test.cc */; };
5491BC721FB44593008B3588 /* FSTIntegrationTestCase.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5491BC711FB44593008B3588 /* FSTIntegrationTestCase.mm */; };
5491BC731FB44593008B3588 /* FSTIntegrationTestCase.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5491BC711FB44593008B3588 /* FSTIntegrationTestCase.mm */; };
54C2294F1FECABAE007D065B /* log_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54C2294E1FECABAE007D065B /* log_test.cc */; };
Expand Down Expand Up @@ -148,7 +149,6 @@
DE51B1FD1F0D492C0013853F /* FSTSpecTests.m in Sources */ = {isa = PBXBuildFile; fileRef = DE51B1991F0D48AC0013853F /* FSTSpecTests.m */; };
DE51B1FE1F0D492C0013853F /* FSTSyncEngineTestDriver.m in Sources */ = {isa = PBXBuildFile; fileRef = DE51B19B1F0D48AC0013853F /* FSTSyncEngineTestDriver.m */; };
DE51B1FF1F0D493A0013853F /* FSTAssertTests.m in Sources */ = {isa = PBXBuildFile; fileRef = DE51B1861F0D48AC0013853F /* FSTAssertTests.m */; };
DE51B2001F0D493A0013853F /* FSTComparisonTests.m in Sources */ = {isa = PBXBuildFile; fileRef = DE51B1871F0D48AC0013853F /* FSTComparisonTests.m */; };
DE51B2011F0D493E0013853F /* FSTHelpers.m in Sources */ = {isa = PBXBuildFile; fileRef = DE51B1891F0D48AC0013853F /* FSTHelpers.m */; };
F104BBD69BC3F0796E3A77C1 /* Pods_Firestore_Tests.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 69F6A10DBD6187489481CD76 /* Pods_Firestore_Tests.framework */; };
/* End PBXBuildFile section */
Expand Down Expand Up @@ -204,6 +204,7 @@
54764FAA1FAA0C320085E60A /* string_util_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = string_util_test.cc; path = ../../Port/string_util_test.cc; sourceTree = "<group>"; };
54764FAE1FAA21B90085E60A /* FSTGoogleTestTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = FSTGoogleTestTests.mm; path = GoogleTest/FSTGoogleTestTests.mm; sourceTree = "<group>"; };
548DB926200D590300E00ABC /* assert_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = assert_test.cc; path = ../../core/test/firebase/firestore/util/assert_test.cc; sourceTree = "<group>"; };
548DB928200D59F600E00ABC /* comparison_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = comparison_test.cc; path = ../../core/test/firebase/firestore/util/comparison_test.cc; sourceTree = "<group>"; };
5491BC711FB44593008B3588 /* FSTIntegrationTestCase.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FSTIntegrationTestCase.mm; sourceTree = "<group>"; };
54C2294E1FECABAE007D065B /* log_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = log_test.cc; path = ../../core/test/firebase/firestore/util/log_test.cc; sourceTree = "<group>"; };
54DA129C1F315EE100DD57A1 /* collection_spec_test.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = collection_spec_test.json; sourceTree = "<group>"; };
Expand Down Expand Up @@ -308,7 +309,6 @@
DE51B1821F0D48AC0013853F /* FSTPathTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FSTPathTests.m; sourceTree = "<group>"; };
DE51B1841F0D48AC0013853F /* FIRGeoPointTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FIRGeoPointTests.m; sourceTree = "<group>"; };
DE51B1861F0D48AC0013853F /* FSTAssertTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FSTAssertTests.m; sourceTree = "<group>"; };
DE51B1871F0D48AC0013853F /* FSTComparisonTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FSTComparisonTests.m; sourceTree = "<group>"; };
DE51B1881F0D48AC0013853F /* FSTHelpers.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = FSTHelpers.h; sourceTree = "<group>"; };
DE51B1891F0D48AC0013853F /* FSTHelpers.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FSTHelpers.m; sourceTree = "<group>"; };
DE51B1941F0D48AC0013853F /* FSTLevelDBSpecTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FSTLevelDBSpecTests.m; sourceTree = "<group>"; };
Expand Down Expand Up @@ -400,6 +400,7 @@
children = (
548DB926200D590300E00ABC /* assert_test.cc */,
54740A521FC913E500713A1A /* autoid_test.cc */,
548DB928200D59F600E00ABC /* comparison_test.cc */,
54C2294E1FECABAE007D065B /* log_test.cc */,
54740A531FC913E500713A1A /* secure_random_test.cc */,
5436F32320008FAD006E51E3 /* string_printf_test.cc */,
Expand Down Expand Up @@ -646,7 +647,6 @@
54E9281E1F33950B00C1953E /* FSTIntegrationTestCase.h */,
5491BC711FB44593008B3588 /* FSTIntegrationTestCase.mm */,
DE51B1861F0D48AC0013853F /* FSTAssertTests.m */,
DE51B1871F0D48AC0013853F /* FSTComparisonTests.m */,
DE51B1881F0D48AC0013853F /* FSTHelpers.h */,
DE51B1891F0D48AC0013853F /* FSTHelpers.m */,
54E9282A1F339CAD00C1953E /* XCTestCase+Await.h */,
Expand Down Expand Up @@ -1210,7 +1210,6 @@
DE2EF0881F3D0B6E003D0CDC /* FSTTreeSortedDictionaryTests.m in Sources */,
DE51B1FD1F0D492C0013853F /* FSTSpecTests.m in Sources */,
ABAEEF4F1FD5F8B100C966CB /* FIRQueryTests.m in Sources */,
DE51B2001F0D493A0013853F /* FSTComparisonTests.m in Sources */,
ABF341051FE860CA00C48322 /* FSTAPIHelpers.m in Sources */,
DE51B1CC1F0D48C00013853F /* FIRGeoPointTests.m in Sources */,
DE51B1E11F0D490D0013853F /* FSTMemoryRemoteDocumentCacheTests.m in Sources */,
Expand Down Expand Up @@ -1276,6 +1275,7 @@
DE51B1E41F0D490D0013853F /* FSTQueryCacheTests.m in Sources */,
DE51B1CD1F0D48CD0013853F /* FSTDatabaseInfoTests.m in Sources */,
AB382F7E1FE03059007CA955 /* FIRFieldPathTests.m in Sources */,
548DB929200D59F600E00ABC /* comparison_test.cc in Sources */,
DE51B1F21F0D49140013853F /* FSTPathTests.m in Sources */,
AB99452C1FE3018D00DFC1E6 /* FIRQuerySnapshotTests.m in Sources */,
54740A571FC914BA00713A1A /* secure_random_test.cc in Sources */,
Expand Down
143 changes: 0 additions & 143 deletions Firestore/Example/Tests/Util/FSTComparisonTests.m

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,14 @@

#import "Firestore/Source/API/FIRGeoPoint+Internal.h"

#import "Firestore/Source/Util/FSTComparison.h"
#import "Firestore/core/src/firebase/firestore/util/comparison.h"

#import "Firestore/Source/Util/FSTUsageValidation.h"

using firebase::firestore::util::DoubleBitwiseEquals;
using firebase::firestore::util::DoubleBitwiseHash;
using firebase::firestore::util::WrapCompare;

NS_ASSUME_NONNULL_BEGIN

@implementation FIRGeoPoint
Expand All @@ -45,11 +50,11 @@ - (instancetype)initWithLatitude:(double)latitude longitude:(double)longitude {
}

- (NSComparisonResult)compare:(FIRGeoPoint *)other {
NSComparisonResult result = FSTCompareDoubles(self.latitude, other.latitude);
NSComparisonResult result = WrapCompare<double>(self.latitude, other.latitude);
if (result != NSOrderedSame) {
return result;
} else {
return FSTCompareDoubles(self.longitude, other.longitude);
return WrapCompare<double>(self.longitude, other.longitude);
}
}

Expand All @@ -67,12 +72,12 @@ - (BOOL)isEqual:(id)other {
return NO;
}
FIRGeoPoint *otherGeoPoint = (FIRGeoPoint *)other;
return FSTDoubleBitwiseEquals(self.latitude, otherGeoPoint.latitude) &&
FSTDoubleBitwiseEquals(self.longitude, otherGeoPoint.longitude);
return DoubleBitwiseEquals(self.latitude, otherGeoPoint.latitude) &&
DoubleBitwiseEquals(self.longitude, otherGeoPoint.longitude);
}

- (NSUInteger)hash {
return 31 * FSTDoubleBitwiseHash(self.latitude) + FSTDoubleBitwiseHash(self.longitude);
return 31 * DoubleBitwiseHash(self.latitude) + DoubleBitwiseHash(self.longitude);
}

/** Implements NSCopying without actually copying because geopoints are immutable. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@

#import "Firestore/Source/Core/FSTTimestamp.h"

#include "Firestore/core/src/firebase/firestore/util/comparison.h"

#import "Firestore/Source/Util/FSTAssert.h"
#import "Firestore/Source/Util/FSTComparison.h"

using firebase::firestore::util::WrapCompare;

NS_ASSUME_NONNULL_BEGIN

Expand Down Expand Up @@ -110,11 +113,11 @@ - (NSString *)ISO8601String {
}

- (NSComparisonResult)compare:(FSTTimestamp *)other {
NSComparisonResult result = FSTCompareInt64s(self.seconds, other.seconds);
NSComparisonResult result = WrapCompare<int64_t>(self.seconds, other.seconds);
if (result != NSOrderedSame) {
return result;
}
return FSTCompareInt32s(self.nanos, other.nanos);
return WrapCompare<int32_t>(self.nanos, other.nanos);
}

@end
Expand Down
4 changes: 2 additions & 2 deletions Firestore/Source/Local/FSTDocumentReference.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ NS_ASSUME_NONNULL_BEGIN
@interface FSTDocumentReference : NSObject <NSCopying>

/** Initializes the document reference with the given key and ID. */
- (instancetype)initWithKey:(FSTDocumentKey *)key ID:(int)ID NS_DESIGNATED_INITIALIZER;
- (instancetype)initWithKey:(FSTDocumentKey *)key ID:(int32_t)ID NS_DESIGNATED_INITIALIZER;

- (instancetype)init NS_UNAVAILABLE;

Expand All @@ -43,7 +43,7 @@ NS_ASSUME_NONNULL_BEGIN
* The targetID of a referring target or the batchID of a referring mutation batch. (Which this
* is depends upon which FSTReferenceSet this reference is a part of.)
*/
@property(nonatomic, assign, readonly) int ID;
@property(nonatomic, assign, readonly) int32_t ID;

@end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@

#import "Firestore/Source/Local/FSTDocumentReference.h"

#include "Firestore/core/src/firebase/firestore/util/comparison.h"

#import "Firestore/Source/Model/FSTDocumentKey.h"
#import "Firestore/Source/Util/FSTComparison.h"

using firebase::firestore::util::WrapCompare;

NS_ASSUME_NONNULL_BEGIN

@implementation FSTDocumentReference

- (instancetype)initWithKey:(FSTDocumentKey *)key ID:(int)ID {
- (instancetype)initWithKey:(FSTDocumentKey *)key ID:(int32_t)ID {
self = [super init];
if (self) {
_key = key;
Expand Down Expand Up @@ -67,13 +70,13 @@ - (id)copyWithZone:(nullable NSZone *)zone {
if (result != NSOrderedSame) {
return result;
}
return FSTCompareInts(left.ID, right.ID);
return WrapCompare<int32_t>(left.ID, right.ID);
};

/** Sorts document references by ID then key. */
const NSComparator FSTDocumentReferenceComparatorByID =
^NSComparisonResult(FSTDocumentReference *left, FSTDocumentReference *right) {
NSComparisonResult result = FSTCompareInts(left.ID, right.ID);
NSComparisonResult result = WrapCompare<int32_t>(left.ID, right.ID);
if (result != NSOrderedSame) {
return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@
#import "Firestore/Source/Model/FSTMutationBatch.h"
#import "Firestore/Source/Model/FSTPath.h"
#import "Firestore/Source/Util/FSTAssert.h"
#import "Firestore/Source/Util/FSTComparison.h"

NS_ASSUME_NONNULL_BEGIN

static const NSComparator NumberComparator = ^NSComparisonResult(NSNumber *left, NSNumber *right) {
return [left compare:right];
};

@interface FSTMemoryMutationQueue ()

/**
Expand Down Expand Up @@ -260,7 +263,7 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID

// Find unique batchIDs referenced by all documents potentially matching the query.
__block FSTImmutableSortedSet<NSNumber *> *uniqueBatchIDs =
[FSTImmutableSortedSet setWithComparator:FSTNumberComparator];
[FSTImmutableSortedSet setWithComparator:NumberComparator];
FSTDocumentReferenceBlock block = ^(FSTDocumentReference *reference, BOOL *stop) {
FSTResourcePath *rowKeyPath = reference.key.path;
if (![prefix isPrefixOfPath:rowKeyPath]) {
Expand Down
Loading

0 comments on commit 39d8252

Please sign in to comment.