From f26f954105e0489b5d9963662791c29f88e3d122 Mon Sep 17 00:00:00 2001 From: Johannes Beck Date: Thu, 2 Jan 2020 19:17:31 +0100 Subject: [PATCH] Fix windows MSVC build. This CL improves the build experience with MSVC: - It adds the build flag '/bigobj' otherwise the build of the unit test fails. - It adds the flag '/wd4267' to suppress signed / unsigned int conversions (size_t to int). - It removes the use of std::aligned_storage from FixedArray. This has been done from the Abseil Team and is in the absl::FixedArray. Those changes has been ported to ceres. This fixes the alignemnt for older MSVC versions due to a bug in the implementation of std::aligned_storage, and prevents the use of the macro '_ENABLE_EXTENDED_ALIGNED_STORAGE' for newer MSVC versions, which is problematic as it could affect user code. - Fix of the fixed array unit test. Due to the use of std::tuple instead of absl::tuple in ceres::internal::FixedArray, the unit test needs to reflect that change as well. - Replaces 'add_definitions' with 'add_compile_options' for compiler flags as suggested by the cmake documentation. Change-Id: I63f08cd6c0a8db8c9931289b909b4deafd75b039 --- CMakeLists.txt | 19 ++++++++++------ include/ceres/internal/fixed_array.h | 33 +++++++++++++++------------- internal/ceres/fixed_array_test.cc | 2 +- 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b25cee30d..f11e57942 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -527,25 +527,30 @@ if (MSVC) # [1] https://msdn.microsoft.com/en-us/library/4hwaceh6.aspx add_definitions("-D_USE_MATH_DEFINES") # Disable signed/unsigned int conversion warnings. - add_definitions("/wd4018") + add_compile_options("/wd4018" "/wd4267") # Disable warning about using struct/class for the same symobl. - add_definitions("/wd4099") + add_compile_options("/wd4099") # Disable warning about the insecurity of using "std::copy". - add_definitions("/wd4996") + add_compile_options("/wd4996") # Disable performance warning about int-to-bool conversion. - add_definitions("/wd4800") + add_compile_options("/wd4800") # Disable performance warning about fopen insecurity. - add_definitions("/wd4996") + add_compile_options("/wd4996") # Disable warning about int64 to int32 conversion. Disabling # this warning may not be correct; needs investigation. # TODO(keir): Investigate these warnings in more detail. - add_definitions("/wd4244") + add_compile_options("/wd4244") # It's not possible to use STL types in DLL interfaces in a portable and # reliable way. However, that's what happens with Google Log and Google Flags # on Windows. MSVC gets upset about this and throws warnings that we can't do # much about. The real solution is to link static versions of Google Log and # Google Test, but that seems tricky on Windows. So, disable the warning. - add_definitions("/wd4251") + add_compile_options("/wd4251") + + # Add bigobj flag otherwise the build would fail due to large object files + # probably resulting from generated headers (like the fixed-size schur + # specializations). + add_compile_options("/bigobj") # Google Flags doesn't have their DLL import/export stuff set up correctly, # which results in linker warnings. This is irrelevant for Ceres, so ignore diff --git a/include/ceres/internal/fixed_array.h b/include/ceres/internal/fixed_array.h index c107dfc95..15481f3f0 100644 --- a/include/ceres/internal/fixed_array.h +++ b/include/ceres/internal/fixed_array.h @@ -163,8 +163,7 @@ class FixedArray { CopyRange(storage_.alloc(), storage_.begin(), first, last); } - // Releases any resources. - ~FixedArray() { + ~FixedArray() noexcept { for (auto* cur = storage_.begin(); cur != storage_.end(); ++cur) { AllocatorTraits::destroy(storage_.alloc(), cur); } @@ -358,7 +357,7 @@ class FixedArray { // error: call to int __builtin___sprintf_chk(etc...) // will always overflow destination buffer [-Werror] // - template ::type, size_t InnerN = std::extent::value> struct StorageElementWrapper { @@ -369,9 +368,6 @@ class FixedArray { typename std::conditional::value, StorageElementWrapper, value_type>::type; - using StorageElementBuffer = - typename std::aligned_storage::type; static pointer AsValueType(pointer ptr) { return ptr; } static pointer AsValueType(StorageElementWrapper* ptr) { @@ -381,25 +377,25 @@ class FixedArray { static_assert(sizeof(StorageElement) == sizeof(value_type), ""); static_assert(alignof(StorageElement) == alignof(value_type), ""); - struct NonEmptyInlinedStorage { - StorageElement* data() { - return reinterpret_cast(inlined_storage_.data()); - } + class NonEmptyInlinedStorage { + public: + StorageElement* data() { return reinterpret_cast(buff_); } + void AnnotateConstruct(size_type) {} + void AnnotateDestruct(size_type) {} // #ifdef ADDRESS_SANITIZER // void* RedzoneBegin() { return &redzone_begin_; } // void* RedzoneEnd() { return &redzone_end_ + 1; } // #endif // ADDRESS_SANITIZER - void AnnotateConstruct(size_type) {} - void AnnotateDestruct(size_type) {} - + private: // ADDRESS_SANITIZER_REDZONE(redzone_begin_); - std::array inlined_storage_; + alignas(StorageElement) char buff_[sizeof(StorageElement[inline_elements])]; // ADDRESS_SANITIZER_REDZONE(redzone_end_); }; - struct EmptyInlinedStorage { + class EmptyInlinedStorage { + public: StorageElement* data() { return nullptr; } void AnnotateConstruct(size_type) {} void AnnotateDestruct(size_type) {} @@ -460,6 +456,13 @@ class FixedArray { Storage storage_; }; +template +constexpr size_t FixedArray::kInlineBytesDefault; + +template +constexpr typename FixedArray::size_type + FixedArray::inline_elements; + } // namespace internal } // namespace ceres diff --git a/internal/ceres/fixed_array_test.cc b/internal/ceres/fixed_array_test.cc index 79ae51132..95cba7fa8 100644 --- a/internal/ceres/fixed_array_test.cc +++ b/internal/ceres/fixed_array_test.cc @@ -477,8 +477,8 @@ TEST(FixedArrayTest, TooBigInlinedSpace) { // Simulate the data members of ceres::internal::FixedArray, a pointer and a // size_t. struct Data { + std::tuple> size_alloc_; TooBig* p; - size_t size; }; // Make sure TooBig objects are not inlined for 0 or default size.