From 3ec1477e74a877109f6b3d586eb20ea7c0155723 Mon Sep 17 00:00:00 2001 From: Teebonne <80053070+Teebonne@users.noreply.github.com> Date: Thu, 8 Sep 2022 19:35:55 +0100 Subject: [PATCH 1/2] Fixing memory leaks and reducing memory usage by switching to a unique_ptr Fixing memory leaks and reducing memory usage by switching to a unique_ptr --- include/xlnt/utils/optional.hpp | 153 ++++++++++---------------------- 1 file changed, 47 insertions(+), 106 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 18d66bd36..27a6e6b92 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -74,28 +74,26 @@ class optional /// Default contructor. is_set() will be false initially. /// optional() noexcept - : has_value_(false) + : optional_ptr() { } /// - /// Constructs this optional with a value. + /// Copy constructs this optional with a value. /// noexcept if T copy ctor is noexcept /// optional(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(ctor_copy_T_noexcept{})) - : has_value_(true) + : optional_ptr(new T(value)) { - new (&storage_) T(value); } /// - /// Constructs this optional with a value. + /// Move constructs this optional with a value. /// noexcept if T move ctor is noexcept /// optional(T &&value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(ctor_move_T_noexcept{})) - : has_value_(true) + : optional_ptr(new T(std::move(value))) { - new (&storage_) T(std::move(value)); } /// @@ -103,12 +101,13 @@ class optional /// noexcept if T copy ctor is noexcept /// optional(const optional &other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(copy_ctor_noexcept{})) - : has_value_(other.has_value_) { - if (has_value_) + if (other.optional_ptr != nullptr) { - new (&storage_) T(other.value_ref()); - } + optional_ptr = std::make_unique(*other.optional_ptr); + } else { + optional_ptr = nullptr; + } } /// @@ -116,13 +115,8 @@ class optional /// noexcept if T move ctor is noexcept /// optional(optional &&other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(move_ctor_noexcept{})) - : has_value_(other.has_value_) + : optional_ptr(std::move(other.optional_ptr)) { - if (has_value_) - { - new (&storage_) T(std::move(other.value_ref())); - other.clear(); - } } /// @@ -131,14 +125,12 @@ class optional /// optional &operator=(const optional &other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{} && clear_noexcept_t{})) { - if (other.has_value_) - { - set(other.value_ref()); - } - else + if (other.optional_ptr != nullptr) { - clear(); - } + optional_ptr.reset(new T(*other.optional_ptr)); + } else { + optional_ptr = nullptr; + } return *this; } @@ -148,15 +140,7 @@ class optional /// optional &operator=(optional &&other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{} && clear_noexcept_t{})) { - if (other.has_value_) - { - set(std::move(other.value_ref())); - other.clear(); - } - else - { - clear(); - } + optional_ptr = std::move(other.optional_ptr); return *this; } @@ -165,7 +149,7 @@ class optional /// ~optional() noexcept // note:: unconditional because msvc freaks out otherwise { - clear(); + optional_ptr.reset(); } /// @@ -174,7 +158,7 @@ class optional /// bool is_set() const noexcept { - return has_value_; + return (optional_ptr != nullptr); } /// @@ -182,49 +166,19 @@ class optional /// 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; - } - else - { - new (&storage_) T(value); - has_value_ = true; - } -#if defined(__GNUC__) && !defined(__clang__) -#pragma GCC diagnostic pop -#endif + optional_ptr.reset(new T(value)); } /// /// Moves the value into the stored value /// + // 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 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 - // 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); - } - else - { - new (&storage_) T(std::move(value)); - has_value_ = true; - } -#if defined(__GNUC__) && !defined(__clang__) -#pragma GCC diagnostic pop -#endif + optional_ptr.reset(new T(std::move(value))); } /// @@ -232,7 +186,7 @@ class optional /// optional &operator=(const T &rhs) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { - set(rhs); + optional_ptr.reset(new T(rhs)); return *this; } @@ -241,7 +195,7 @@ class optional /// optional &operator=(T &&rhs) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{})) { - set(std::move(rhs)); + optional_ptr.reset(new T(std::move(rhs))); return *this; } @@ -250,11 +204,7 @@ class optional /// void clear() noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(clear_noexcept_t{})) { - if (has_value_) - { - reinterpret_cast(&storage_)->~T(); - } - has_value_ = false; + optional_ptr.reset(); } /// @@ -263,12 +213,11 @@ class optional /// T &get() { - if (!has_value_) + if (optional_ptr == nullptr) { throw invalid_attribute(); } - - return value_ref(); + return *optional_ptr; } /// @@ -277,12 +226,11 @@ class optional /// const T &get() const { - if (!has_value_) + if (optional_ptr == nullptr) { throw invalid_attribute(); } - - return value_ref(); + return *optional_ptr; } /// @@ -292,16 +240,21 @@ class optional /// bool operator==(const optional &other) const noexcept { - if (has_value_ != other.has_value_) + if (optional_ptr == nullptr) { - return false; - } - if (!has_value_) - { - return true; - } - // equality is overloaded to provide fuzzy equality when T is a fp number - return compare_equal(value_ref(), other.value_ref()); + if (other.optional_ptr == nullptr) { + return true; + } else { + return false; + }; + } else { + if (other.optional_ptr == nullptr) { + return false; + } else { + // equality is overloaded to provide fuzzy equality when T is a fp number + return compare_equal(*optional_ptr, *other.optional_ptr); + }; + }; } /// @@ -315,19 +268,7 @@ class optional } private: - // helpers for getting a T out of storage - T &value_ref() noexcept - { - return *reinterpret_cast(&storage_); - } - - const T &value_ref() const noexcept - { - return *reinterpret_cast(&storage_); - } - - bool has_value_; - typename std::aligned_storage::type storage_; + std::unique_ptr optional_ptr; }; #ifdef XLNT_NOEXCEPT_VALUE_COMPAT From ea15e071a011d8647ba11cc21046aab6d7d2e782 Mon Sep 17 00:00:00 2001 From: Teebonne <80053070+Teebonne@users.noreply.github.com> Date: Sun, 9 Oct 2022 18:56:06 +0100 Subject: [PATCH 2/2] Update optional.hpp --- include/xlnt/utils/optional.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 27a6e6b92..ab9c9a7df 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -27,6 +27,7 @@ #include "xlnt/utils/numeric.hpp" #include "xlnt/xlnt_config.hpp" #include +#include namespace xlnt {