From e1ace703fbf4458d9e887ebf30050b1a0482a2d2 Mon Sep 17 00:00:00 2001 From: Gil Date: Wed, 11 Jul 2018 15:27:50 -0700 Subject: Fix Firestore source errors under VS2017 (#1515) * Project file updates from sync_project.rb * Fix misc compile errors under VS2017 * Fix util/hashing under VS2017 std::hash is not just a pass through in Microsoft's STL. * Disable unsafe code warnings in VS2017 ... where comparing against a reference implementation that has no easy safe equivalent. * Handle drive letters in paths on Windows --- .../Example/Firestore.xcodeproj/project.pbxproj | 4 +- .../firestore/immutable/sorted_map_iterator.h | 4 +- Firestore/core/src/firebase/firestore/util/bits.h | 14 +++---- .../core/src/firebase/firestore/util/hard_assert.h | 34 ++++++++------- .../core/src/firebase/firestore/util/hashing.h | 9 ++-- Firestore/core/src/firebase/firestore/util/path.cc | 47 ++++++++++++++++----- .../core/src/firebase/firestore/util/status.cc | 44 +++++++++++++------- .../firestore/immutable/sorted_map_test.cc | 7 ++++ .../test/firebase/firestore/util/hashing_test.cc | 48 ++++++++++++++-------- .../test/firebase/firestore/util/strerror_test.cc | 11 +++++ 10 files changed, 155 insertions(+), 67 deletions(-) (limited to 'Firestore') diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index 6729eab..08e4926 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -610,8 +610,8 @@ 546854A720A3681B004BDBD5 /* remote */ = { isa = PBXGroup; children = ( - B6D1B68420E2AB1A00B35856 /* exponential_backoff_test.cc */, 546854A820A36867004BDBD5 /* datastore_test.cc */, + B6D1B68420E2AB1A00B35856 /* exponential_backoff_test.cc */, 61F72C5520BC48FD001A68CB /* serializer_test.cc */, ); path = remote; @@ -1750,7 +1750,6 @@ 5492E03F2021401F00B64F25 /* FSTHelpers.mm in Sources */, DE2EF0861F3D0B6E003D0CDC /* FSTImmutableSortedDictionary+Testing.m in Sources */, DE2EF0871F3D0B6E003D0CDC /* FSTImmutableSortedSet+Testing.m in Sources */, - B6D1B68520E2AB1B00B35856 /* exponential_backoff_test.cc in Sources */, 5491BC721FB44593008B3588 /* FSTIntegrationTestCase.mm in Sources */, 5492E0A72021552D00B64F25 /* FSTLevelDBKeyTests.mm in Sources */, 5492E0A82021552D00B64F25 /* FSTLevelDBLocalStoreTests.mm in Sources */, @@ -1808,6 +1807,7 @@ B6FB468E208F9BAB00554BA2 /* executor_libdispatch_test.mm in Sources */, B6FB468F208F9BAE00554BA2 /* executor_std_test.cc in Sources */, B6FB4690208F9BB300554BA2 /* executor_test.cc in Sources */, + B6D1B68520E2AB1B00B35856 /* exponential_backoff_test.cc in Sources */, 549CCA5720A36E1F00BCEB75 /* field_mask_test.cc in Sources */, B686F2AF2023DDEE0028D6BE /* field_path_test.cc in Sources */, 54A0352620A3AED0003E0143 /* field_transform_test.mm in Sources */, diff --git a/Firestore/core/src/firebase/firestore/immutable/sorted_map_iterator.h b/Firestore/core/src/firebase/firestore/immutable/sorted_map_iterator.h index 960e3f6..20c11a7 100644 --- a/Firestore/core/src/firebase/firestore/immutable/sorted_map_iterator.h +++ b/Firestore/core/src/firebase/firestore/immutable/sorted_map_iterator.h @@ -119,7 +119,9 @@ class SortedMapIterator { pointer get() const { switch (tag_) { case Tag::Array: - return array_iter_; + // std::array::iterator is not guaranteed to be a bare pointer but will + // be a RandomAccessIterator which does have operator*(). + return &*array_iter_; case Tag::Tree: return tree_iter_.get(); } diff --git a/Firestore/core/src/firebase/firestore/util/bits.h b/Firestore/core/src/firebase/firestore/util/bits.h index 94a018f..591c306 100644 --- a/Firestore/core/src/firebase/firestore/util/bits.h +++ b/Firestore/core/src/firebase/firestore/util/bits.h @@ -24,13 +24,13 @@ #include -class Bits_Port32_Test; -class Bits_Port64_Test; - namespace firebase { namespace firestore { namespace util { +class Bits_Port32_Test; +class Bits_Port64_Test; + class Bits { public: /** Return floor(log2(n)) for positive integer n. Returns -1 iff n == 0. */ @@ -81,9 +81,9 @@ inline int Bits::Log2FloorNonZero64(uint64_t n) { return 63 ^ __builtin_clzll(n); } -#elif defined(COMPILER_MSVC) +#elif defined(_MSC_VER) -inline int Bits::Log2FloorNonZero(uint32 n) { +inline int Bits::Log2FloorNonZero(uint32_t n) { #ifdef _M_IX86 _asm { bsr ebx, n @@ -95,7 +95,7 @@ inline int Bits::Log2FloorNonZero(uint32 n) { #endif } -inline int Bits::Log2Floor(uint32 n) { +inline int Bits::Log2Floor(uint32_t n) { #ifdef _M_IX86 _asm { xor ebx, ebx @@ -120,7 +120,7 @@ inline int Bits::Log2FloorNonZero64(uint64_t n) { return Bits::Log2FloorNonZero64_Portable(n); } -#else // !__GNUC__ && !COMPILER_MSVC +#else // !__GNUC__ && !_MSC_VER inline int Bits::Log2Floor64(uint64_t n) { return Bits::Log2Floor64_Portable(n); diff --git a/Firestore/core/src/firebase/firestore/util/hard_assert.h b/Firestore/core/src/firebase/firestore/util/hard_assert.h index e60d71a..7b66f33 100644 --- a/Firestore/core/src/firebase/firestore/util/hard_assert.h +++ b/Firestore/core/src/firebase/firestore/util/hard_assert.h @@ -21,6 +21,12 @@ #include "Firestore/core/src/firebase/firestore/util/string_format.h" +#if defined(_MSC_VER) +#define FIRESTORE_FUNCTION_NAME __FUNCSIG__ +#else +#define FIRESTORE_FUNCTION_NAME __PRETTY_FUNCTION__ +#endif + /** * Fails the current function if the given condition is false. * @@ -30,14 +36,14 @@ * @param format (optional) A format string suitable for util::StringFormat. * @param ... format arguments to pass to util::StringFormat. */ -#define HARD_ASSERT(condition, ...) \ - do { \ - if (!(condition)) { \ - std::string _message = \ - firebase::firestore::util::StringFormat(__VA_ARGS__); \ - firebase::firestore::util::internal::Fail( \ - __FILE__, __PRETTY_FUNCTION__, __LINE__, _message, #condition); \ - } \ +#define HARD_ASSERT(condition, ...) \ + do { \ + if (!(condition)) { \ + std::string _message = \ + firebase::firestore::util::StringFormat(__VA_ARGS__); \ + firebase::firestore::util::internal::Fail( \ + __FILE__, FIRESTORE_FUNCTION_NAME, __LINE__, _message, #condition); \ + } \ } while (0) /** @@ -48,12 +54,12 @@ * @param format A format string suitable for util::StringFormat. * @param ... format arguments to pass to util::StringFormat. */ -#define HARD_FAIL(...) \ - do { \ - std::string _failure = \ - firebase::firestore::util::StringFormat(__VA_ARGS__); \ - firebase::firestore::util::internal::Fail(__FILE__, __PRETTY_FUNCTION__, \ - __LINE__, _failure); \ +#define HARD_FAIL(...) \ + do { \ + std::string _failure = \ + firebase::firestore::util::StringFormat(__VA_ARGS__); \ + firebase::firestore::util::internal::Fail( \ + __FILE__, FIRESTORE_FUNCTION_NAME, __LINE__, _failure); \ } while (0) /** diff --git a/Firestore/core/src/firebase/firestore/util/hashing.h b/Firestore/core/src/firebase/firestore/util/hashing.h index 21c0bd6..5219d64 100644 --- a/Firestore/core/src/firebase/firestore/util/hashing.h +++ b/Firestore/core/src/firebase/firestore/util/hashing.h @@ -82,7 +82,8 @@ struct has_std_hash { * `decltype(std::hash{}(std::declval()))`. */ template -using std_hash_type = typename std::enable_if{}, size_t>::type; +using std_hash_type = + typename std::enable_if::value, size_t>::type; /** * Combines a hash_value with whatever accumulated state there is so far. @@ -151,9 +152,11 @@ auto RankedInvokeHash(const Range& range, HashChoice<2>) size_t size = 0; for (auto&& element : range) { ++size; - result = Combine(result, InvokeHash(element)); + size_t piece = InvokeHash(element); + result = Combine(result, piece); } - result = Combine(result, size); + size_t size_hash = InvokeHash(size); + result = Combine(result, size_hash); return result; } diff --git a/Firestore/core/src/firebase/firestore/util/path.cc b/Firestore/core/src/firebase/firestore/util/path.cc index 940f12a..0da3b23 100644 --- a/Firestore/core/src/firebase/firestore/util/path.cc +++ b/Firestore/core/src/firebase/firestore/util/path.cc @@ -16,14 +16,45 @@ #include "Firestore/core/src/firebase/firestore/util/path.h" +#include "absl/strings/ascii.h" +#include "absl/strings/string_view.h" + namespace firebase { namespace firestore { namespace util { +namespace { + +static constexpr absl::string_view::size_type npos = absl::string_view::npos; + +/** Returns the given path with its leading drive letter removed. */ +inline absl::string_view StripDriveLetter(absl::string_view path) { +#if defined(_WIN32) + if (path.size() >= 2 && path[1] == ':' && absl::ascii_isalpha(path[0])) { + return path.substr(2); + } + return path; + +#else + return path; +#endif // defined(_WIN32) +} + +/** Returns true if the given character is a pathname separator. */ +inline bool IsSeparator(char c) { +#if defined(_WIN32) + return c == '/' || c == '\\'; +#else + return c == '/'; +#endif // defined(_WIN32) +} + +} // namespace + absl::string_view Path::Basename(absl::string_view pathname) { size_t slash = pathname.find_last_of('/'); - if (slash == absl::string_view::npos) { + if (slash == npos) { // No path separator found => the whole string. return pathname; } @@ -35,7 +66,7 @@ absl::string_view Path::Basename(absl::string_view pathname) { absl::string_view Path::Dirname(absl::string_view pathname) { size_t last_slash = pathname.find_last_of('/'); - if (last_slash == absl::string_view::npos) { + if (last_slash == npos) { // No path separator found => empty string. Conformance with POSIX would // have us return "." here. return pathname.substr(0, 0); @@ -43,7 +74,7 @@ absl::string_view Path::Dirname(absl::string_view pathname) { // Collapse runs of slashes. size_t nonslash = pathname.find_last_not_of('/', last_slash); - if (nonslash == absl::string_view::npos) { + if (nonslash == npos) { // All characters preceding the last path separator are slashes return pathname.substr(0, 1); } @@ -55,12 +86,8 @@ absl::string_view Path::Dirname(absl::string_view pathname) { } bool Path::IsAbsolute(absl::string_view path) { -#if defined(_WIN32) -#error "Handle drive letters" - -#else - return !path.empty() && path.front() == '/'; -#endif + path = StripDriveLetter(path); + return !path.empty() && IsSeparator(path.front()); } void Path::JoinAppend(std::string* base, absl::string_view path) { @@ -69,7 +96,7 @@ void Path::JoinAppend(std::string* base, absl::string_view path) { } else { size_t nonslash = base->find_last_not_of('/'); - if (nonslash != std::string::npos) { + if (nonslash != npos) { base->resize(nonslash + 1); base->push_back('/'); } diff --git a/Firestore/core/src/firebase/firestore/util/status.cc b/Firestore/core/src/firebase/firestore/util/status.cc index 3e8585a..beaa88a 100644 --- a/Firestore/core/src/firebase/firestore/util/status.cc +++ b/Firestore/core/src/firebase/firestore/util/status.cc @@ -100,18 +100,24 @@ static FirestoreErrorCode CodeForErrno(int errno_code) { #if defined(EISNAM) case EISNAM: // Is a named type file #endif - case ENOTBLK: // Block device required - case ENOTCONN: // The socket is not connected - case EPIPE: // Broken pipe +#if defined(ENOTBLK) + case ENOTBLK: // Block device required +#endif + case ENOTCONN: // The socket is not connected + case EPIPE: // Broken pipe +#if defined(ESHUTDOWN) case ESHUTDOWN: // Cannot send after transport endpoint shutdown - case ETXTBSY: // Text file busy +#endif + case ETXTBSY: // Text file busy #if defined(EUNATCH) case EUNATCH: // Protocol driver not attached #endif return FirestoreErrorCode::FailedPrecondition; - case ENOSPC: // No space left on device - case EDQUOT: // Disk quota exceeded + case ENOSPC: // No space left on device +#if defined(EDQUOT) + case EDQUOT: // Disk quota exceeded +#endif case EMFILE: // Too many open files case EMLINK: // Too many links case ENFILE: // Too many open files in system @@ -119,7 +125,9 @@ static FirestoreErrorCode CodeForErrno(int errno_code) { case ENODATA: // No message is available on the STREAM read queue case ENOMEM: // Not enough space case ENOSR: // No STREAM resources - case EUSERS: // Too many users +#if defined(EUSERS) + case EUSERS: // Too many users +#endif return FirestoreErrorCode::ResourceExhausted; #if defined(ECHRNG) @@ -133,13 +141,17 @@ static FirestoreErrorCode CodeForErrno(int errno_code) { #if defined(ENOPKG) case ENOPKG: // Package not installed #endif - case ENOSYS: // Function not implemented - case ENOTSUP: // Operation not supported - case EAFNOSUPPORT: // Address family not supported - case EPFNOSUPPORT: // Protocol family not supported + case ENOSYS: // Function not implemented + case ENOTSUP: // Operation not supported + case EAFNOSUPPORT: // Address family not supported +#if defined(EPFNOSUPPORT) + case EPFNOSUPPORT: // Protocol family not supported +#endif case EPROTONOSUPPORT: // Protocol not supported +#if defined(ESOCKTNOSUPPORT) case ESOCKTNOSUPPORT: // Socket type not supported - case EXDEV: // Improper link +#endif + case EXDEV: // Improper link return FirestoreErrorCode::Unimplemented; case EAGAIN: // Resource temporarily unavailable @@ -150,7 +162,9 @@ static FirestoreErrorCode CodeForErrno(int errno_code) { case ECONNABORTED: // Connection aborted case ECONNRESET: // Connection reset case EINTR: // Interrupted function call - case EHOSTDOWN: // Host is down +#if defined(EHOSTDOWN) + case EHOSTDOWN: // Host is down +#endif case EHOSTUNREACH: // Host is unreachable case ENETDOWN: // Network is down case ENETRESET: // Connection aborted by network @@ -163,7 +177,9 @@ static FirestoreErrorCode CodeForErrno(int errno_code) { return FirestoreErrorCode::Unavailable; case EDEADLK: // Resource deadlock avoided - case ESTALE: // Stale file handle +#if defined(ESTALE) + case ESTALE: // Stale file handle +#endif return FirestoreErrorCode::Aborted; case ECANCELED: // Operation cancelled diff --git a/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc b/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc index 75353d9..acd0642 100644 --- a/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc +++ b/Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc @@ -14,6 +14,13 @@ * limitations under the License. */ +// Disable warnings in the MSVC standard library about the use of std::mismatch. +// There's no guaranteed safe way to use it in C++11 but the test verifies that +// our implementation matches the standard so we need to call it. +#if defined(_MSC_VER) +#define _SCL_SECURE_NO_WARNINGS 1 +#endif + #include "Firestore/core/src/firebase/firestore/immutable/sorted_map.h" #include diff --git a/Firestore/core/test/firebase/firestore/util/hashing_test.cc b/Firestore/core/test/firebase/firestore/util/hashing_test.cc index 2c5c2f7..d762bbe 100644 --- a/Firestore/core/test/firebase/firestore/util/hashing_test.cc +++ b/Firestore/core/test/firebase/firestore/util/hashing_test.cc @@ -28,7 +28,7 @@ namespace util { struct HasHashMember { size_t Hash() const { - return 42; + return 42u; } }; @@ -66,22 +66,23 @@ TEST(HashingTest, StringView) { // that, but that requires an explicit specialization. Since we're only // defining this for compatibility with Objective-C and not really sensitive // to performance or hash quality here, this is good enough. - size_t expected = 'a'; - expected = 31u * expected + 1; + size_t expected = std::hash{}('a'); + expected = 31u * expected + std::hash{}(1); ASSERT_EQ(expected, Hash(absl::string_view{"a"})); } TEST(HashingTest, SizeT) { - ASSERT_EQ(42u, Hash(size_t{42u})); + size_t expected = std::hash{}(42u); + ASSERT_EQ(expected, Hash(size_t{42u})); } TEST(HashingTest, Array) { int values[] = {0, 1, 2}; - size_t expected = 0; - expected = 31 * expected + 1; - expected = 31 * expected + 2; - expected = 31 * expected + 3; // length of array + size_t expected = std::hash{}(0); + expected = 31u * expected + std::hash{}(1); + expected = 31u * expected + std::hash{}(2); + expected = 31u * expected + std::hash{}(3); // length of array ASSERT_EQ(expected, Hash(values)); } @@ -91,7 +92,10 @@ TEST(HashingTest, HasHashMember) { TEST(HashingTest, RangeOfStdHashable) { std::vector values{42}; - ASSERT_EQ(31u * 42u + 1, Hash(values)); + + size_t expected = std::hash{}(42); + expected = 31u * expected + std::hash{}(1); // length of array + ASSERT_EQ(expected, Hash(values)); std::vector values_leading_zero{0, 42}; std::vector values_trailing_zero{42, 0}; @@ -103,18 +107,30 @@ TEST(HashingTest, RangeOfStdHashable) { TEST(HashingTest, RangeOfHashMember) { std::vector values{HasHashMember{}}; - ASSERT_EQ(31u * 42u + 1, Hash(values)); + + // We trust the underlying Hash() member to do its thing, so unlike the other + // examples, the 42u here is not run through std::hash{}(). + size_t expected = 42u; + expected = 31u * expected + std::hash{}(1); // length of array + ASSERT_EQ(expected, Hash(values)); } TEST(HashingTest, Composite) { // Verify the result ends up as if hand-rolled - EXPECT_EQ(1u, Hash(1)); - EXPECT_EQ(31u, Hash(1, 0)); - EXPECT_EQ(31u * 31u, Hash(1, 0, 0)); + EXPECT_EQ(std::hash{}(1), Hash(1)); + + size_t expected = std::hash{}(1); + expected = 31 * expected + std::hash{}(0); + EXPECT_EQ(expected, Hash(1, 0)); + + expected = std::hash{}(1); + expected = 31 * expected + std::hash{}(0); + expected = 31 * expected + std::hash{}(0); + EXPECT_EQ(expected, Hash(1, 0, 0)); - size_t expected = Hash(1); - expected = 31 * expected + Hash(2); - expected = 31 * expected + Hash(3); + expected = Hash(1); + expected = 31u * expected + Hash(2); + expected = 31u * expected + Hash(3); EXPECT_EQ(expected, Hash(1, 2, 3)); } diff --git a/Firestore/core/test/firebase/firestore/util/strerror_test.cc b/Firestore/core/test/firebase/firestore/util/strerror_test.cc index 854cb08..e6e64a4 100644 --- a/Firestore/core/test/firebase/firestore/util/strerror_test.cc +++ b/Firestore/core/test/firebase/firestore/util/strerror_test.cc @@ -23,9 +23,20 @@ namespace firestore { namespace util { TEST(StrErrorTest, ValidErrorCode) { +#if defined(_MSC_VER) +#pragma warning(push) + // strerror is unsafe generally, but it's used here as the simplest possible + // reference implementation. +#pragma warning(disable : 4996) +#endif + errno = EAGAIN; EXPECT_EQ(StrError(EINTR), strerror(EINTR)); EXPECT_EQ(errno, EAGAIN); + +#if defined(_MSC_VER) +#pragma warning(pop) +#endif } TEST(StrErrorTest, InvalidErrorCode) { -- cgit v1.2.3