From bb546e19885ae084823e0315e93564a44c0a8257 Mon Sep 17 00:00:00 2001 From: Gil Date: Fri, 1 Jun 2018 13:42:47 -0700 Subject: Fix Firestore compilation under Xcode < 9.2 (#1367) * Don't rely on specialization failure to determine when std::hash is unavailable. Instead manually declare the conditions under which std::hash should be defined. * Fix detection of Objective-C classes in Xcode < 9.2 std::is_base_of{} is false there so the overloads defined for Objective-C types weren't getting enabled. * Add explicit tests for StringFormat using Objective-C objects * Add explicit tests for HasStdHash --- .../src/firebase/firestore/util/CMakeLists.txt | 1 + .../core/src/firebase/firestore/util/hashing.h | 39 +++++++++- .../src/firebase/firestore/util/string_format.h | 28 +++---- .../core/src/firebase/firestore/util/type_traits.h | 90 ++++++++++++++++++++++ .../test/firebase/firestore/util/CMakeLists.txt | 9 +++ .../test/firebase/firestore/util/hashing_test.cc | 18 +++++ .../firestore/util/string_format_apple_test.mm | 60 +++++++++++++++ .../firestore/util/type_traits_apple_test.mm | 50 ++++++++++++ 8 files changed, 274 insertions(+), 21 deletions(-) create mode 100644 Firestore/core/src/firebase/firestore/util/type_traits.h create mode 100644 Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm create mode 100644 Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm (limited to 'Firestore/core') diff --git a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt index 043713f..ed3a301 100644 --- a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt +++ b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt @@ -200,6 +200,7 @@ cc_library( statusor_internals.h string_util.cc string_util.h + type_traits.h DEPENDS absl_base firebase_firestore_util_base diff --git a/Firestore/core/src/firebase/firestore/util/hashing.h b/Firestore/core/src/firebase/firestore/util/hashing.h index d8058c8..21c0bd6 100644 --- a/Firestore/core/src/firebase/firestore/util/hashing.h +++ b/Firestore/core/src/firebase/firestore/util/hashing.h @@ -18,6 +18,7 @@ #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_HASHING_H_ #include +#include #include namespace firebase { @@ -48,6 +49,41 @@ namespace util { namespace impl { +/** + * A type trait that identifies whether or not std::hash is available for a + * given type. + * + * This type should not be necessary since specialization failure on an + * expression like `decltype(std::hash{}(value)` should be enough to disable + * overloads that require `std::hash` to be defined but unfortunately some + * standard libraries ship with std::hash defined for all types that only + * fail later (e.g. via static_assert). One such implementation is the libc++ + * that ships with Xcode 8.3.3, which is a supported platform. + */ +template +struct has_std_hash { + // There may be other types for which std::hash is defined but they don't + // matter for our purposes. + enum { + value = std::is_arithmetic{} || std::is_pointer{} || + std::is_same{} + }; + + constexpr operator bool() const { + return value; + } +}; + +/** + * A type that's equivalent to size_t if std::hash is defined or a compile + * error otherwise. + * + * This is effectively just a safe implementation of + * `decltype(std::hash{}(std::declval()))`. + */ +template +using std_hash_type = typename std::enable_if{}, size_t>::type; + /** * Combines a hash_value with whatever accumulated state there is so far. */ @@ -100,8 +136,7 @@ auto RankedInvokeHash(const K& value, HashChoice<0>) -> decltype(value.Hash()) { * @return The result of `std::hash{}(value)` */ template -auto RankedInvokeHash(const K& value, HashChoice<1>) - -> decltype(std::hash{}(value)) { +std_hash_type RankedInvokeHash(const K& value, HashChoice<1>) { return std::hash{}(value); } diff --git a/Firestore/core/src/firebase/firestore/util/string_format.h b/Firestore/core/src/firebase/firestore/util/string_format.h index d691984..01776a9 100644 --- a/Firestore/core/src/firebase/firestore/util/string_format.h +++ b/Firestore/core/src/firebase/firestore/util/string_format.h @@ -22,6 +22,7 @@ #include #include "Firestore/core/src/firebase/firestore/util/string_apple.h" +#include "Firestore/core/src/firebase/firestore/util/type_traits.h" #include "absl/base/attributes.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" @@ -43,7 +44,7 @@ template struct FormatChoice : FormatChoice {}; template <> -struct FormatChoice<4> {}; +struct FormatChoice<5> {}; } // namespace internal @@ -87,8 +88,8 @@ class FormatArg : public absl::AlphaNum { */ template < typename T, - typename = typename std::enable_if{}>::type> - FormatArg(T* object, internal::FormatChoice<0>) + typename = typename std::enable_if{}>::type> + FormatArg(T object, internal::FormatChoice<1>) : AlphaNum{MakeStringView([object description])} { } @@ -96,20 +97,9 @@ class FormatArg : public absl::AlphaNum { * Creates a FormatArg from any Objective-C Class type. Objective-C Class * types are a special struct that aren't of a type derived from NSObject. */ - FormatArg(Class object, internal::FormatChoice<0>) + FormatArg(Class object, internal::FormatChoice<1>) : AlphaNum{MakeStringView(NSStringFromClass(object))} { } - - /** - * Creates a FormatArg from any id pointer. Note that instances of `id` - * (which means "pointer conforming to the protocol Foo") do not match this - * without first casting to type `id`. There's no way to express a template of - * `id` since `id` isn't actually a C++ template and `id` isn't a - * parameterized C++ class. - */ - FormatArg(id object, internal::FormatChoice<0>) - : AlphaNum{MakeStringView([object description])} { - } #endif /** @@ -117,7 +107,7 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(std::nullptr_t, internal::FormatChoice<1>) : AlphaNum{"null"} { + FormatArg(std::nullptr_t, internal::FormatChoice<2>) : AlphaNum{"null"} { } /** @@ -125,7 +115,7 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(const char* string_value, internal::FormatChoice<2>) + FormatArg(const char* string_value, internal::FormatChoice<3>) : AlphaNum{string_value == nullptr ? "null" : string_value} { } @@ -134,7 +124,7 @@ class FormatArg : public absl::AlphaNum { * hexidecimal integer literal. */ template - FormatArg(T* pointer_value, internal::FormatChoice<3>) + FormatArg(T* pointer_value, internal::FormatChoice<4>) : AlphaNum{absl::Hex{reinterpret_cast(pointer_value)}} { } @@ -143,7 +133,7 @@ class FormatArg : public absl::AlphaNum { * absl::AlphaNum accepts. */ template - FormatArg(T&& value, internal::FormatChoice<4>) + FormatArg(T&& value, internal::FormatChoice<5>) : AlphaNum{std::forward(value)} { } }; diff --git a/Firestore/core/src/firebase/firestore/util/type_traits.h b/Firestore/core/src/firebase/firestore/util/type_traits.h new file mode 100644 index 0000000..52feb6b --- /dev/null +++ b/Firestore/core/src/firebase/firestore/util/type_traits.h @@ -0,0 +1,90 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_ +#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_ + +#if __OBJC__ +#import // for id +#endif + +#include + +namespace firebase { +namespace firestore { +namespace util { + +#if __OBJC__ + +/** + * A type trait that identifies whether or not the given pointer points to an + * Objective-C object. + * + * is_objective_c_pointer::value == true + * is_objective_c_pointer*>::value == true + * + * // id is a dynamically typed pointer to an Objective-C object. + * is_objective_c_pointer::value == true + * + * // pointers to C++ classes are not Objective-C pointers. + * is_objective_c_pointer::value == false + * is_objective_c_pointer::value == false + * is_objective_c_pointer>::value == false + */ +template +struct is_objective_c_pointer { + private: + using yes_type = char (&)[10]; + using no_type = char (&)[1]; + + /** + * A non-existent function declared to produce a pointer to type T (which is + * consistent with the way Objective-C objects are referenced). + * + * Note that there is no definition for this function but that's okay because + * we only need it to reason about the function's type at compile type. + */ + static T Pointer(); + + static yes_type Choose(id value); + static no_type Choose(...); + + public: + using value_type = bool; + + enum { value = sizeof(Choose(Pointer())) == sizeof(yes_type) }; + + constexpr operator bool() const { + return value; + } + + constexpr bool operator()() const { + return value; + } +}; + +// Hard-code the answer for `void` because you can't pass arguments of type +// `void` to another function. +template <> +struct is_objective_c_pointer : public std::false_type {}; + +#endif // __OBJC__ + +} // namespace util +} // namespace firestore +} // namespace firebase + +#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_ diff --git a/Firestore/core/test/firebase/firestore/util/CMakeLists.txt b/Firestore/core/test/firebase/firestore/util/CMakeLists.txt index bcb1c84..c07a4ea 100644 --- a/Firestore/core/test/firebase/firestore/util/CMakeLists.txt +++ b/Firestore/core/test/firebase/firestore/util/CMakeLists.txt @@ -137,3 +137,12 @@ cc_test( firebase_firestore_util gmock ) + +if(APPLE) + target_sources( + firebase_firestore_util_test + PUBLIC + string_format_apple_test.mm + type_traits_apple_test.mm + ) +endif() diff --git a/Firestore/core/test/firebase/firestore/util/hashing_test.cc b/Firestore/core/test/firebase/firestore/util/hashing_test.cc index e5d9ff8..2c5c2f7 100644 --- a/Firestore/core/test/firebase/firestore/util/hashing_test.cc +++ b/Firestore/core/test/firebase/firestore/util/hashing_test.cc @@ -16,6 +16,9 @@ #include "Firestore/core/src/firebase/firestore/util/hashing.h" +#include +#include + #include "absl/strings/string_view.h" #include "gtest/gtest.h" @@ -29,6 +32,21 @@ struct HasHashMember { } }; +TEST(HashingTest, HasStdHash) { + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + + struct Foo {}; + EXPECT_FALSE(impl::has_std_hash::value); + EXPECT_FALSE(impl::has_std_hash::value); + EXPECT_FALSE((impl::has_std_hash>::value)); +} + TEST(HashingTest, Int) { ASSERT_EQ(std::hash{}(0), Hash(0)); } diff --git a/Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm b/Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm new file mode 100644 index 0000000..f0bcd35 --- /dev/null +++ b/Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm @@ -0,0 +1,60 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/firebase/firestore/util/string_format.h" + +#import + +#include "gtest/gtest.h" + +@interface FSTDescribable : NSObject +@end + +@implementation FSTDescribable + +- (NSString*)description { + return @"description"; +} + +@end + +namespace firebase { +namespace firestore { +namespace util { + +TEST(StringFormatTest, NSString) { + EXPECT_EQ("Hello World", StringFormat("Hello %s", @"World")); + + NSString* hello = [NSString stringWithUTF8String:"Hello"]; + EXPECT_EQ("Hello World", StringFormat("%s World", hello)); + + // NOLINTNEXTLINE false positive on "string" + NSMutableString* world = [NSMutableString string]; + [world appendString:@"World"]; + EXPECT_EQ("Hello World", StringFormat("Hello %s", world)); +} + +TEST(StringFormatTest, FSTDescribable) { + FSTDescribable* desc = [[FSTDescribable alloc] init]; + EXPECT_EQ("Hello description", StringFormat("Hello %s", desc)); + + id desc_id = desc; + EXPECT_EQ("Hello description", StringFormat("Hello %s", desc_id)); +} + +} // namespace util +} // namespace firestore +} // namespace firebase diff --git a/Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm b/Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm new file mode 100644 index 0000000..dfb03bb --- /dev/null +++ b/Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm @@ -0,0 +1,50 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/firebase/firestore/util/type_traits.h" + +#import +#import +#import + +#include "gtest/gtest.h" + +namespace firebase { +namespace firestore { +namespace util { + +TEST(TypeTraitsTest, IsObjectiveCPointer) { + static_assert(is_objective_c_pointer{}, "NSObject"); + static_assert(is_objective_c_pointer{}, "NSString"); + static_assert(is_objective_c_pointer*>{}, + "NSArray"); + + static_assert(is_objective_c_pointer{}, "id"); + static_assert(is_objective_c_pointer>{}, "id"); + + static_assert(!is_objective_c_pointer{}, "int*"); + static_assert(!is_objective_c_pointer{}, "void*"); + static_assert(!is_objective_c_pointer{}, "int"); + static_assert(!is_objective_c_pointer{}, "void"); + + struct Foo {}; + static_assert(!is_objective_c_pointer{}, "Foo"); + static_assert(!is_objective_c_pointer{}, "Foo"); +} + +} // namespace util +} // namespace firestore +} // namespace firebase -- cgit v1.2.3