From eda102ee9dc1c544fb5404e625c561f71443d145 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:22:30 +1200 Subject: [PATCH 1/9] testing current optional implementation against target behaviour Issue #300 -- NOTE: construction from no_default currently wont compile so is commented out (L:113) -- NOTE: dll import/export of template classes is probably unnecessary (the header is already required) and doesn't work in the general case (explicit instantiations are required for at least MSVC) --- include/xlnt/utils/optional.hpp | 12 +- tests/utils/optional_tests.cpp | 304 ++++++++++++++++++++++++++++++++ 2 files changed, 315 insertions(+), 1 deletion(-) create mode 100644 tests/utils/optional_tests.cpp diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 143c38ec..1978009b 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -34,7 +34,7 @@ namespace xlnt { /// within the optional class. /// template -class XLNT_API optional +class optional { public: /// @@ -135,6 +135,16 @@ public: return value_ == other.value_; } + /// + /// Returns false if neither this nor other have a value + /// or both have a value and those values are equal according to + /// their equality operator. + /// + bool operator!=(const optional &other) const + { + return !operator==(other); + } + private: bool has_value_; T value_; diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp new file mode 100644 index 00000000..b17cbecb --- /dev/null +++ b/tests/utils/optional_tests.cpp @@ -0,0 +1,304 @@ +// Copyright (c) 2014-2018 Thomas Fussell +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE +// +// @license: http://www.opensource.org/licenses/mit-license.php +// @author: see AUTHORS file + +#pragma once + +#include +#include + +// test helpers +namespace { +// increments count when constructed, decrements when destructed +// use to ensure correct ctor/dtor pairing +struct alive_count +{ + alive_count() + { + ++count; + } + + alive_count(const alive_count &) + { + ++count; + } + + alive_count(alive_count &&) + { + ++count; + } + + ~alive_count() + { + --count; + } + + alive_count &operator=(const alive_count &) = default; + alive_count &operator=(alive_count &&) = default; + + static int count; +}; +int alive_count::count = 0; + +// implicitly convertible from int +struct convertible +{ + // implicit construction from int + convertible(int i) + : val(i) + { + } + + int val; +}; + +// default ctor deleted +struct no_default +{ + no_default() = delete; + int i; +}; +} // namespace + +class optional_test_suite : public test_suite +{ +public: + optional_test_suite() + : test_suite() + { + register_test(test_ctor); + register_test(test_copy_ctor); + register_test(test_move_ctor); + register_test(test_copy_assign); + register_test(test_move_assign); + register_test(test_set_and_get); + register_test(test_equality); + register_test(test_const); + } + + void test_ctor() + { + // default + xlnt::optional opt1; + xlnt_assert(!opt1.is_set()); + // value + const int test_val = 3; + xlnt::optional opt2(test_val); + xlnt_assert(opt2.is_set()); + xlnt_assert_equals(opt2.get(), test_val); + // converting + xlnt::optional opt3(test_val); + xlnt_assert(opt3.is_set()); + xlnt_assert_equals(opt3.get().val, test_val); + //// no default ctor + //xlnt::optional no_def_opt; + } + + void test_copy_ctor() + { + { // copy behaviour + xlnt::optional opt1; + xlnt::optional opt2(opt1); + xlnt_assert_equals(opt1, opt2); + + const int test_val = 123; + xlnt::optional opt3(test_val); + xlnt::optional opt4(opt3); + xlnt_assert_equals(opt3, opt4); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt2(opt1); + xlnt_assert_equals(2, alive_count::count); + } + xlnt_assert_equals(1, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_move_ctor() + { + { // move behaviour + xlnt::optional opt1; + xlnt::optional opt2(std::move(opt1)); + xlnt_assert_equals(opt2, xlnt::optional{}); // can't test against opt1 so use a temporary + + const int test_val = 123; + xlnt::optional opt3(test_val); + xlnt::optional opt4(std::move(opt3)); + xlnt_assert(opt4.is_set()); // moved to optional contains the value + xlnt_assert_equals(opt4.get(), test_val); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt2(std::move(opt1)); + xlnt_assert_equals(1, alive_count::count); // opt1 is in a no-value state + } + xlnt_assert_equals(0, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_copy_assign() + { + { // copy assign behaviour + xlnt::optional opt1; + xlnt::optional opt_assign1; // to actually test assignment, the value needs to be already created. using '=' is not enough + opt_assign1 = opt1; + xlnt_assert_equals(opt1, opt_assign1); + + const int test_val = 123; + xlnt::optional opt2(test_val); + xlnt::optional opt_assign2; + opt_assign2 = opt2; + xlnt_assert_equals(opt2, opt_assign2); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt_assign1; + opt_assign1 = opt1; + xlnt_assert_equals(2, alive_count::count); + } + xlnt_assert_equals(1, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_move_assign() + { + { // copy assign behaviour + xlnt::optional opt1; + xlnt::optional opt_assign1; // to actually test assignment, the value needs to be already created. using '=' is not enough + opt_assign1 = std::move(opt1); + xlnt_assert_equals(opt_assign1, xlnt::optional{}); // can't test against opt1 so use a temporary + + const int test_val = 123; + xlnt::optional opt2(test_val); + xlnt::optional opt_assign2; + opt_assign2 = std::move(opt2); + xlnt_assert(opt_assign2.is_set()); // moved to optional contains the value + xlnt_assert_equals(opt_assign2.get(), test_val); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt_assign1; + opt_assign1 = std::move(opt1); + xlnt_assert_equals(1, alive_count::count); // opt1 is in a no-value state + } + xlnt_assert_equals(0, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_set_and_get() + { + { + xlnt::optional test_opt; + xlnt_assert(!test_opt.is_set()); + xlnt_assert_throws(test_opt.get(), xlnt::invalid_attribute); + // set + const int test_val1 = 321; + test_opt.set(test_val1); + xlnt_assert(test_opt.is_set()); + xlnt_assert_equals(test_opt.get(), test_val1); + // set again + const int test_val2 = 123; + test_opt.set(test_val2); + xlnt_assert(test_opt.is_set()); + xlnt_assert_equals(test_opt.get(), test_val2); + // clear + test_opt.clear(); + xlnt_assert(!test_opt.is_set()); + xlnt_assert_throws(test_opt.get(), xlnt::invalid_attribute); + // set again + const int test_val3 = 3; + test_opt.set(test_val3); + xlnt_assert(test_opt.is_set()); + xlnt_assert_equals(test_opt.get(), test_val3); + } + { // lifetime checks + xlnt::optional test_opt; + xlnt_assert_equals(0, alive_count::count); + test_opt.set(alive_count()); + xlnt_assert_equals(1, alive_count::count); + test_opt.set(alive_count()); // reassignment doesn't somehow skip the dtor + xlnt_assert_equals(1, alive_count::count); + test_opt.clear(); + xlnt_assert_equals(0, alive_count::count); + } + } + + void test_equality() + { + xlnt::optional test_opt1; + xlnt::optional test_opt2; + // no value opts compare equal + xlnt_assert(test_opt1 == test_opt2); + xlnt_assert(!(test_opt1 != test_opt2)); + xlnt_assert(test_opt2 == test_opt1); + xlnt_assert(!(test_opt2 != test_opt1)); + // value compares false with no value + const int test_val = 1; + test_opt1.set(test_val); + xlnt_assert(test_opt1 != test_opt2); + xlnt_assert(!(test_opt1 == test_opt2)); + xlnt_assert(test_opt2 != test_opt1); + xlnt_assert(!(test_opt2 == test_opt1)); + // value compares false with a different value + const int test_val2 = 2; + test_opt2.set(test_val2); + xlnt_assert(test_opt1 != test_opt2); + xlnt_assert(!(test_opt1 == test_opt2)); + xlnt_assert(test_opt2 != test_opt1); + xlnt_assert(!(test_opt2 == test_opt1)); + // value compares equal with same value + test_opt2.set(test_val); + xlnt_assert(test_opt1 == test_opt2); + xlnt_assert(!(test_opt1 != test_opt2)); + xlnt_assert(test_opt2 == test_opt1); + xlnt_assert(!(test_opt2 != test_opt1)); + } + + void test_const() + { + // functions on a const optional + const int test_val = 1; + const xlnt::optional opt(test_val); + xlnt_assert(opt.is_set()); + xlnt_assert(opt.get() == test_val); + + xlnt::optional opt2(test_val); + xlnt_assert(opt == opt2); + xlnt_assert(opt2 == opt); + xlnt_assert(!(opt != opt2)); + xlnt_assert(!(opt2 != opt)); + } +}; +static optional_test_suite x; \ No newline at end of file From ad69e7bf11aeaf4b5aa990dccca66c4467d7591a Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:22:43 +1200 Subject: [PATCH 2/9] test for overloaded operator=(T) Issue #300 --- tests/utils/optional_tests.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp index b17cbecb..7af72a20 100644 --- a/tests/utils/optional_tests.cpp +++ b/tests/utils/optional_tests.cpp @@ -242,6 +242,10 @@ public: test_opt.set(test_val3); xlnt_assert(test_opt.is_set()); xlnt_assert_equals(test_opt.get(), test_val3); + // operator= set + xlnt::optional test_opt2; + test_opt2 = test_val1; + xlnt_assert_equals(test_opt2.get(), test_val1); } { // lifetime checks xlnt::optional test_opt; From 34e8f274def67e166ad57196be24ee8d45345d50 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:22:54 +1200 Subject: [PATCH 3/9] optional exception check, primarily adding noexcept to signatures -- also reordered the assignments in set/clear to ensure "has_value_" doesn't change if the assignment operator of T throws --- include/xlnt/utils/optional.hpp | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 1978009b..03415304 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -40,14 +40,17 @@ public: /// /// Default contructor. is_set() will be false initially. /// - optional() : has_value_(false), value_(T()) + optional() noexcept(std::is_nothrow_default_constructible{}) + : has_value_(false), value_(T()) { } /// /// Constructs this optional with a value. + /// noexcept if T copy ctor is noexcept /// - optional(const T &value) : has_value_(true), value_(value) + optional(const T &value) noexcept(std::is_nothrow_copy_constructible{}) + : has_value_(true), value_(value) { } @@ -55,7 +58,7 @@ public: /// Returns true if this object currently has a value set. This should /// be called before accessing the value with optional::get(). /// - bool is_set() const + bool is_set() const noexcept { return has_value_; } @@ -63,10 +66,10 @@ public: /// /// Sets the value to value. /// - void set(const T &value) + void set(const T &value) noexcept(std::is_nothrow_assignable{}) { - has_value_ = true; value_ = value; + has_value_ = true; } /// @@ -101,20 +104,18 @@ public: /// Resets the internal value using its default constructor. After this is /// called, is_set() will return false until a new value is provided. /// - void clear() + void clear() noexcept(std::is_nothrow_assignable{} && std::is_nothrow_default_constructible{}) { - has_value_ = false; value_ = T(); + has_value_ = false; } /// /// Assignment operator. Equivalent to setting the value using optional::set. /// - optional &operator=(const T &rhs) + optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) { - has_value_ = true; - value_ = rhs; - + set(rhs); return *this; } @@ -123,9 +124,10 @@ public: /// or both have a value and those values are equal according to /// their equality operator. /// - bool operator==(const optional &other) const + bool operator==(const optional &other) const noexcept { - if (has_value_ != other.has_value_) { + if (has_value_ != other.has_value_) + { return false; } if (!has_value_) @@ -140,7 +142,7 @@ public: /// or both have a value and those values are equal according to /// their equality operator. /// - bool operator!=(const optional &other) const + bool operator!=(const optional &other) const noexcept { return !operator==(other); } From 07d648fe8b6e27d2c78f1e4076a5b726b5d0ae84 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 7 Jul 2018 21:05:50 +1200 Subject: [PATCH 4/9] Implementation of optional using std::aligned_storage Issue #300 -- test for no default constructor required enabled -- Passes all tests locally --- include/xlnt/utils/optional.hpp | 190 +++++++++++++++++++++++++++----- tests/utils/optional_tests.cpp | 4 +- 2 files changed, 162 insertions(+), 32 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 03415304..75fd4ac5 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -40,8 +40,8 @@ public: /// /// Default contructor. is_set() will be false initially. /// - optional() noexcept(std::is_nothrow_default_constructible{}) - : has_value_(false), value_(T()) + optional() noexcept + : has_value_(false) { } @@ -50,8 +50,89 @@ public: /// noexcept if T copy ctor is noexcept /// optional(const T &value) noexcept(std::is_nothrow_copy_constructible{}) - : has_value_(true), value_(value) + : has_value_(true) { + new (&storage_) T(value); + } + + /// + /// Constructs this optional with a value. + /// noexcept if T move ctor is noexcept + /// + optional(T &&value) noexcept(std::is_nothrow_move_constructible{}) + : has_value_(true) + { + new (&storage_) T(std::move(value)); + } + + /// + /// Copy constructs this optional from other + /// noexcept if T copy ctor is noexcept + /// + optional(const optional& other) noexcept(std::is_nothrow_copy_constructible{}) + : has_value_(other.has_value_) + { + if (has_value_) + { + new (&storage_) T(other.value_ref()); + } + } + + /// + /// Move constructs this optional from other. Clears the value from other if set + /// noexcept if T move ctor is noexcept + /// + optional(optional &&other) noexcept(std::is_nothrow_move_constructible{}) + : has_value_(other.has_value_) + { + if (has_value_) + { + new (&storage_) T(std::move(other.value_ref())); + other.clear(); + } + } + + /// + /// Copy assignment of this optional from other + /// noexcept if set and clear are noexcept for T& + /// + optional &operator=(const optional &other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + { + if (other.has_value_) + { + set(other.value_ref()); + } + else + { + clear(); + } + return *this; + } + + /// + /// Move assignment of this optional from other + /// noexcept if set and clear are noexcept for T&& + /// + optional &operator=(optional &&other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + { + if (other.has_value_) + { + set(std::move(other.value_ref())); + other.clear(); + } + else + { + clear(); + } + return *this; + } + + /// + /// Destructor cleans up the T instance if set + /// + ~optional() noexcept // note:: unconditional because msvc freaks out otherwise + { + clear(); } /// @@ -64,12 +145,69 @@ public: } /// - /// Sets the value to value. + /// Copies the value into the stored value /// - void set(const T &value) noexcept(std::is_nothrow_assignable{}) + void set(const T &value) noexcept(std::is_nothrow_copy_constructible{} && std::is_nothrow_assignable{}) { - value_ = value; - has_value_ = true; + if (has_value_) + { + value_ref() = value; + } + else + { + new (&storage_) T(value); + has_value_ = true; + } + } + + /// + /// Moves the value into the stored value + /// + void set(T &&value) noexcept(std::is_nothrow_move_constructible{} && std::is_nothrow_move_assignable{}) + { + // note seperate overload for two reasons (as opposed to perfect forwarding) + // 1. have to deal with implicit conversions internally with perfect forwarding + // 2. have to deal with the noexcept specfiers for all the different variations + // overload is just far and away the simpler solution + if (has_value_) + { + value_ref() = std::move(value); + } + else + { + new (&storage_) T(std::move(value)); + has_value_ = true; + } + } + + /// + /// Assignment operator overload. Equivalent to setting the value using optional::set. + /// + optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) + { + set(rhs); + return *this; + } + + /// + /// Assignment operator overload. Equivalent to setting the value using optional::set. + /// + optional &operator=(T &&rhs) noexcept(noexcept(set(std::declval()))) + { + set(std::move(rhs)); + return *this; + } + + /// + /// After this is called, is_set() will return false until a new value is provided. + /// + void clear() noexcept(std::is_nothrow_destructible{}) + { + if (has_value_) + { + reinterpret_cast(&storage_)->~T(); + } + has_value_ = false; } /// @@ -83,7 +221,7 @@ public: throw invalid_attribute(); } - return value_; + return value_ref(); } /// @@ -97,26 +235,7 @@ public: throw invalid_attribute(); } - return value_; - } - - /// - /// Resets the internal value using its default constructor. After this is - /// called, is_set() will return false until a new value is provided. - /// - void clear() noexcept(std::is_nothrow_assignable{} && std::is_nothrow_default_constructible{}) - { - value_ = T(); - has_value_ = false; - } - - /// - /// Assignment operator. Equivalent to setting the value using optional::set. - /// - optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) - { - set(rhs); - return *this; + return value_ref(); } /// @@ -134,7 +253,7 @@ public: { return true; } - return value_ == other.value_; + return value_ref() == other.value_ref(); } /// @@ -148,8 +267,19 @@ public: } private: + // helpers for getting a T out of storage + T & value_ref() + { + return *reinterpret_cast(&storage_); + } + + const T &value_ref() const + { + return *reinterpret_cast(&storage_); + } + bool has_value_; - T value_; + typename std::aligned_storage::type storage_; }; } // namespace xlnt diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp index 7af72a20..1b32af87 100644 --- a/tests/utils/optional_tests.cpp +++ b/tests/utils/optional_tests.cpp @@ -109,8 +109,8 @@ public: xlnt::optional opt3(test_val); xlnt_assert(opt3.is_set()); xlnt_assert_equals(opt3.get().val, test_val); - //// no default ctor - //xlnt::optional no_def_opt; + // no default ctor + xlnt::optional no_def_opt; } void test_copy_ctor() From b004d0863c64d2fa0e6bada7bd6fe373baee393c Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:26:34 +1200 Subject: [PATCH 5/9] address CI build failure Issue #300 Previous CI fix didn't work, try with type aliasing instead Issue #300 This reverts commit c87c0e39756062fbc761698e96978b0936005b0e. gcc 6 ok, msvc 14 choked Issue #300 noexcept(condition) doesn't seem to work on msvs 2015 Issue #300 Merge remote-tracking branch 'origin/dev-optional-no-default-ctor' into dev-resolve-simple-issues ensure uniqueness (uglify) and undefine compatibility macro --- include/xlnt/utils/optional.hpp | 53 ++++++++++++++++++++++++--------- tests/utils/optional_tests.cpp | 3 +- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 75fd4ac5..bc02407d 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -25,6 +25,7 @@ #include #include +#include namespace xlnt { @@ -36,6 +37,26 @@ namespace xlnt { template class optional { +#if _MSC_VER <= 1900 // v14, visual studio 2015 +#define XLNT_NOEXCEPT_VALUE_COMPAT(...) (false) + using ctor_copy_T_noexcept = std::false_type; + using ctor_move_T_noexcept = std::false_type; + using copy_ctor_noexcept = ctor_copy_T_noexcept; + using move_ctor_noexcept = ctor_move_T_noexcept; + using set_copy_noexcept_t = std::false_type; + using set_move_noexcept_t = std::false_type; + using clear_noexcept_t = std::false_type; +#else +#define XLNT_NOEXCEPT_VALUE_COMPAT(...) (__VA_ARGS__) + using ctor_copy_T_noexcept = typename std::conditional{}, std::true_type, std::false_type>::type; + using ctor_move_T_noexcept = typename std::conditional{}, std::true_type, std::false_type>::type; + using copy_ctor_noexcept = ctor_copy_T_noexcept; + using move_ctor_noexcept = ctor_move_T_noexcept; + using set_copy_noexcept_t = typename std::conditional{} && std::is_nothrow_assignable{}, std::true_type, std::false_type>::type; + using set_move_noexcept_t = typename std::conditional{} && std::is_nothrow_move_assignable{}, std::true_type, std::false_type>::type; + using clear_noexcept_t = typename std::conditional{}, std::true_type, std::false_type>::type; +#endif + public: /// /// Default contructor. is_set() will be false initially. @@ -49,7 +70,7 @@ public: /// Constructs this optional with a value. /// noexcept if T copy ctor is noexcept /// - optional(const T &value) noexcept(std::is_nothrow_copy_constructible{}) + optional(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(ctor_copy_T_noexcept{})) : has_value_(true) { new (&storage_) T(value); @@ -59,7 +80,7 @@ public: /// Constructs this optional with a value. /// noexcept if T move ctor is noexcept /// - optional(T &&value) noexcept(std::is_nothrow_move_constructible{}) + optional(T &&value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(ctor_move_T_noexcept{})) : has_value_(true) { new (&storage_) T(std::move(value)); @@ -69,7 +90,7 @@ public: /// Copy constructs this optional from other /// noexcept if T copy ctor is noexcept /// - optional(const optional& other) noexcept(std::is_nothrow_copy_constructible{}) + optional(const optional &other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(copy_ctor_noexcept{})) : has_value_(other.has_value_) { if (has_value_) @@ -82,7 +103,7 @@ public: /// Move constructs this optional from other. Clears the value from other if set /// noexcept if T move ctor is noexcept /// - optional(optional &&other) noexcept(std::is_nothrow_move_constructible{}) + optional(optional &&other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(move_ctor_noexcept{})) : has_value_(other.has_value_) { if (has_value_) @@ -96,7 +117,7 @@ public: /// Copy assignment of this optional from other /// noexcept if set and clear are noexcept for T& /// - optional &operator=(const optional &other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + optional &operator=(const optional &other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{} && clear_noexcept_t{})) { if (other.has_value_) { @@ -113,7 +134,7 @@ public: /// Move assignment of this optional from other /// noexcept if set and clear are noexcept for T&& /// - optional &operator=(optional &&other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + optional &operator=(optional &&other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{} && clear_noexcept_t{})) { if (other.has_value_) { @@ -147,7 +168,7 @@ public: /// /// Copies the value into the stored value /// - void set(const T &value) noexcept(std::is_nothrow_copy_constructible{} && std::is_nothrow_assignable{}) + void set(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { if (has_value_) { @@ -163,7 +184,7 @@ public: /// /// Moves the value into the stored value /// - void set(T &&value) noexcept(std::is_nothrow_move_constructible{} && std::is_nothrow_move_assignable{}) + void set(T &&value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{})) { // note seperate overload for two reasons (as opposed to perfect forwarding) // 1. have to deal with implicit conversions internally with perfect forwarding @@ -183,7 +204,7 @@ public: /// /// Assignment operator overload. Equivalent to setting the value using optional::set. /// - optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) + optional &operator=(const T &rhs) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { set(rhs); return *this; @@ -192,7 +213,7 @@ public: /// /// Assignment operator overload. Equivalent to setting the value using optional::set. /// - optional &operator=(T &&rhs) noexcept(noexcept(set(std::declval()))) + optional &operator=(T &&rhs) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{})) { set(std::move(rhs)); return *this; @@ -201,7 +222,7 @@ public: /// /// After this is called, is_set() will return false until a new value is provided. /// - void clear() noexcept(std::is_nothrow_destructible{}) + void clear() noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(clear_noexcept_t{})) { if (has_value_) { @@ -268,12 +289,12 @@ public: private: // helpers for getting a T out of storage - T & value_ref() + T &value_ref() noexcept { return *reinterpret_cast(&storage_); } - const T &value_ref() const + const T &value_ref() const noexcept { return *reinterpret_cast(&storage_); } @@ -282,4 +303,8 @@ private: typename std::aligned_storage::type storage_; }; -} // namespace xlnt +#ifdef XLNT_NOEXCEPT_VALUE_COMPAT +#undef XLNT_NOEXCEPT_VALUE_COMPAT +#endif + +} // namespace xlnt \ No newline at end of file diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp index 1b32af87..6313e167 100644 --- a/tests/utils/optional_tests.cpp +++ b/tests/utils/optional_tests.cpp @@ -21,9 +21,8 @@ // @license: http://www.opensource.org/licenses/mit-license.php // @author: see AUTHORS file -#pragma once - #include +#include #include // test helpers From ad24d9485db9d9af7b2b3210c7601872fc1a6ba5 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Wed, 11 Jul 2018 12:27:18 +1200 Subject: [PATCH 6/9] Resolve CI warning about using an uninitialised variable --- include/xlnt/utils/optional.hpp | 6 +++ source/detail/serialization/xlsx_consumer.cpp | 42 ++++++++++--------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index bc02407d..e019f316 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -170,6 +170,8 @@ public: /// void set(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" if (has_value_) { value_ref() = value; @@ -179,6 +181,7 @@ public: new (&storage_) T(value); has_value_ = true; } +#pragma GCC diagnostic pop } /// @@ -190,6 +193,8 @@ public: // 1. have to deal with implicit conversions internally with perfect forwarding // 2. have to deal with the noexcept specfiers for all the different variations // overload is just far and away the simpler solution +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" if (has_value_) { value_ref() = std::move(value); @@ -199,6 +204,7 @@ public: new (&storage_) T(std::move(value)); has_value_ = true; } +#pragma GCC diagnostic pop } /// diff --git a/source/detail/serialization/xlsx_consumer.cpp b/source/detail/serialization/xlsx_consumer.cpp index 336ddaa3..a634bf7b 100644 --- a/source/detail/serialization/xlsx_consumer.cpp +++ b/source/detail/serialization/xlsx_consumer.cpp @@ -387,7 +387,7 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) props.sync_horizontal.set(parser().attribute("syncHorizontal")); } if (parser().attribute_present("syncVertical")) - {// optional, boolean, false + { // optional, boolean, false props.sync_vertical.set(parser().attribute("syncVertical")); } if (parser().attribute_present("syncRef")) @@ -399,11 +399,11 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) props.transition_evaluation.set(parser().attribute("transitionEvaluation")); } if (parser().attribute_present("transitionEntry")) - {// optional, boolean, false + { // optional, boolean, false props.transition_entry.set(parser().attribute("transitionEntry")); } if (parser().attribute_present("published")) - {// optional, boolean, true + { // optional, boolean, true props.published.set(parser().attribute("published")); } if (parser().attribute_present("codeName")) @@ -411,11 +411,11 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) props.code_name.set(parser().attribute("codeName")); } if (parser().attribute_present("filterMode")) - {// optional, boolean, false + { // optional, boolean, false props.filter_mode.set(parser().attribute("filterMode")); } if (parser().attribute_present("enableFormatConditionsCalculation")) - {// optional, boolean, true + { // optional, boolean, true props.enable_format_condition_calculation.set(parser().attribute("enableFormatConditionsCalculation")); } ws.d_->sheet_properties_.set(props); @@ -606,19 +606,22 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) auto min = static_cast(std::stoull(parser().attribute("min"))); auto max = static_cast(std::stoull(parser().attribute("max"))); - optional width; - - if (parser().attribute_present("width")) - { - width = (parser().attribute("width") * 7 - 5) / 7; - } - - optional column_style; - - if (parser().attribute_present("style")) - { - column_style = parser().attribute("style"); - } + // avoid uninitialised warnings in GCC by using a lambda to make the conditional initialisation + optional width = [](xml::parser &p) -> xlnt::optional { + if (p.attribute_present("width")) + { + return (p.attribute("width") * 7 - 5) / 7; + } + return xlnt::optional(); + }(parser()); + // avoid uninitialised warnings in GCC by using a lambda to make the conditional initialisation + optional column_style = [](xml::parser &p) -> xlnt::optional { + if (p.attribute_present("style")) + { + return p.attribute("style"); + } + return xlnt::optional(); + }(parser()); auto custom = parser().attribute_present("customWidth") ? is_true(parser().attribute("customWidth")) @@ -1843,7 +1846,8 @@ void xlsx_consumer::read_office_document(const std::string &content_type) // CT_ target_.d_->sheet_title_rel_id_map_.end(), [&](const std::pair &p) { return p.second == worksheet_rel.id(); - })->first; + }) + ->first; auto id = sheet_title_id_map_[title]; auto index = sheet_title_index_map_[title]; From fede2d31680b9ddf218ccae1c4129ee29923d587 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 21 Jul 2018 12:33:58 +1200 Subject: [PATCH 7/9] fix Clang CI warnings --- include/xlnt/utils/optional.hpp | 9 +-------- source/CMakeLists.txt | 1 + 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index e019f316..f19bc286 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -37,15 +37,8 @@ namespace xlnt { template class optional { -#if _MSC_VER <= 1900 // v14, visual studio 2015 +#if defined(_MSC_VER) && _MSC_VER <= 1900 // v14, visual studio 2015 #define XLNT_NOEXCEPT_VALUE_COMPAT(...) (false) - using ctor_copy_T_noexcept = std::false_type; - using ctor_move_T_noexcept = std::false_type; - using copy_ctor_noexcept = ctor_copy_T_noexcept; - using move_ctor_noexcept = ctor_move_T_noexcept; - using set_copy_noexcept_t = std::false_type; - using set_move_noexcept_t = std::false_type; - using clear_noexcept_t = std::false_type; #else #define XLNT_NOEXCEPT_VALUE_COMPAT(...) (__VA_ARGS__) using ctor_copy_T_noexcept = typename std::conditional{}, std::true_type, std::false_type>::type; diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 2ea048d3..11e3593c 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -41,6 +41,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-padded") # ignore padding warnings set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-documentation-unknown-command") # ignore unknown commands in Javadoc-style comments set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas") # ignore Windows and GCC pragmas + set CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-warning-option") # ignore Windows and GCC pragmas set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-float-equal") # don't warn on uses of == for fp types set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-newline-eof") # no longer an issue with post-c++11 standards which mandate include add a newline if neccesary set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-covered-switch-default") # default is often added to switches for completeness or to cover future alternatives From 6562c41ae1e16f4bdfcfab5c48c8523c7dba5480 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 21 Jul 2018 12:38:33 +1200 Subject: [PATCH 8/9] fix typo in Cmake lists --- source/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 11e3593c..5c924e84 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -41,7 +41,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-padded") # ignore padding warnings set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-documentation-unknown-command") # ignore unknown commands in Javadoc-style comments set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas") # ignore Windows and GCC pragmas - set CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-warning-option") # ignore Windows and GCC pragmas + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-warning-option") # ignore Windows and GCC pragmas set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-float-equal") # don't warn on uses of == for fp types set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-newline-eof") # no longer an issue with post-c++11 standards which mandate include add a newline if neccesary set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-covered-switch-default") # default is often added to switches for completeness or to cover future alternatives From 9e981abe0549a458dc918fe1cdb75532473ba22e Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 21 Jul 2018 13:30:52 +1200 Subject: [PATCH 9/9] Only add diagnostics to GCC, Clang doesn't know the warning --- include/xlnt/utils/optional.hpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index f19bc286..8248ebd2 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -163,8 +163,10 @@ public: /// void set(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif if (has_value_) { value_ref() = value; @@ -174,7 +176,9 @@ public: new (&storage_) T(value); has_value_ = true; } +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic pop +#endif } /// @@ -186,8 +190,10 @@ public: // 1. have to deal with implicit conversions internally with perfect forwarding // 2. have to deal with the noexcept specfiers for all the different variations // overload is just far and away the simpler solution +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif if (has_value_) { value_ref() = std::move(value); @@ -197,7 +203,9 @@ public: new (&storage_) T(std::move(value)); has_value_ = true; } +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic pop +#endif } ///