Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: only destruct elements no longer in use when erasing #43

Merged
merged 2 commits into from
Mar 11, 2024
Merged

fix: only destruct elements no longer in use when erasing #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21e219a
fix: destruct elements no longer in use only when erasing
tearfur Mar 9, 2024
133bb26
chore: reformat with ClangFormat
tearfur Mar 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 51 additions & 54 deletions include/small/vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <small/detail/traits/enable_allocator_from_this.hpp>
#include <small/detail/traits/is_range.hpp>
#include <small/detail/traits/is_relocatable.hpp>
#include <algorithm>
#include <cassert>
#include <cstddef>
#include <cstdlib>
Expand Down Expand Up @@ -182,8 +183,8 @@ namespace small {

/// \brief True if we should just copy the inline storage
static constexpr bool should_copy_inline
= std::is_trivially_copyable_v<
value_type> && sizeof(inline_storage_type) <= cache_line_size / 2;
= std::is_trivially_copyable_v<value_type>
&& sizeof(inline_storage_type) <= cache_line_size / 2;

/// \brief True if we are using the std::allocator
static constexpr bool using_std_allocator = std::
Expand All @@ -200,8 +201,8 @@ namespace small {

/// \brief Use memcpy to copy items
/// If type is relocatable, we just use memcpy
static constexpr bool relocate_use_memcpy
= is_relocatable_v<T> && using_std_allocator;
static constexpr bool relocate_use_memcpy = is_relocatable_v<T>
&& using_std_allocator;

public:
/// \section Rule of five constructors
Expand Down Expand Up @@ -283,7 +284,8 @@ namespace small {
} else {
auto n = rhs.size();
if constexpr (std::is_trivially_move_constructible_v<
value_type>) {
value_type>)
{
std::memcpy(
(void *) begin().base(),
(void *) rhs.begin().base(),
Expand Down Expand Up @@ -367,7 +369,8 @@ namespace small {
} else {
const size_t n = rhs.size();
if constexpr (std::is_trivially_move_assignable_v<
value_type>) {
value_type>)
{
std::memcpy(
(void *) data_.buffer(),
(void *) rhs.data_.buffer(),
Expand Down Expand Up @@ -422,7 +425,7 @@ namespace small {
assert(invariants());
}

public /* constructors */:
public /* constructors */:
/// \section Initialization constructors

/// \brief Construct empty small array
Expand All @@ -440,20 +443,14 @@ namespace small {
constexpr explicit vector(
size_type n,
const allocator_type &alloc = allocator_type())
: vector(
n,
[&](void *p) { new (p) value_type(); },
alloc) {}
: vector(n, [&](void *p) { new (p) value_type(); }, alloc) {}

/// \brief Construct small array with size n and fill with single value
constexpr vector(
size_type n,
const value_type &value,
const allocator_type &alloc = allocator_type())
: vector(
n,
[&](void *p) { new (p) value_type(value); },
alloc) {}
: vector(n, [&](void *p) { new (p) value_type(value); }, alloc) {}

/// \brief Construct small array from a pair of iterators
template <class Iterator>
Expand Down Expand Up @@ -554,7 +551,7 @@ namespace small {
constexpr void
swap(vector &rhs) noexcept(
std::is_nothrow_move_constructible_v<value_type>
&&std::is_nothrow_swappable_v<value_type>) {
&& std::is_nothrow_swappable_v<value_type>) {
// Allow ADL on swap for our value_type.
using std::swap;

Expand Down Expand Up @@ -667,7 +664,7 @@ namespace small {
assert(invariants());
}

public /* iterators */:
public /* iterators */:
/// \brief Get iterator to first element
constexpr iterator
begin() noexcept {
Expand Down Expand Up @@ -740,7 +737,7 @@ namespace small {
return std::reverse_iterator<const_iterator>(cbegin());
}

public /* capacity */:
public /* capacity */:
/// \brief Get small array size
[[nodiscard]] constexpr size_type
size() const noexcept {
Expand Down Expand Up @@ -815,7 +812,7 @@ namespace small {
tmp.swap(*this);
}

public /* element access */:
public /* element access */:
/// \brief Get reference to n-th element in small array
constexpr reference
operator[](size_type n) {
Expand Down Expand Up @@ -891,7 +888,7 @@ namespace small {
return this->is_external() ? data_.heap() : data_.buffer();
}

public /* modifiers */:
public /* modifiers */:
/// \brief Copy element to end of small array
constexpr void
push_back(const value_type &v) {
Expand All @@ -910,7 +907,8 @@ namespace small {
emplace_back(Args &&...args) {
// Handle inline vector
if (size_ < num_inline_elements) {
auto* ptr = new (data_.buffer() + size_) value_type(std::forward<Args>(args)...);
auto *ptr = new (data_.buffer() + size_)
value_type(std::forward<Args>(args)...);
this->increment_internal_size(1);
return *ptr;
} else {
Expand All @@ -924,12 +922,9 @@ namespace small {
const bool needs_to_grow = old_capacity == old_size;
if (needs_to_grow) {
// Internal vector
make_size(
old_size + 1,
[&](void *p) {
make_size(old_size + 1, [&](void *p) {
new (p) value_type(std::forward<Args>(args)...);
},
old_size);
}, old_size);
} else {
// External vector
new (data_.heap() + old_size)
Expand Down Expand Up @@ -982,10 +977,9 @@ namespace small {
auto old_size = size();
const bool must_grow = capacity() == old_size;
if (must_grow) {
make_size(
old_size + 1,
[&x](void *ptr) { new (ptr) value_type(std::move(x)); },
offset);
make_size(old_size + 1, [&x](void *ptr) {
new (ptr) value_type(std::move(x));
}, offset);
this->increment_internal_size(1);
} else {
shift_right_and_construct(
Expand Down Expand Up @@ -1068,33 +1062,38 @@ namespace small {
if (first == last) {
return unconst(first);
}

// Directly destroy elements before mem moving
if constexpr (!std::is_trivially_destructible_v<T>) {
for (auto it = first; it != last; ++it) {
it->~value_type();
}
}


const auto n_erase = last - first;
if constexpr (is_relocatable_v<value_type> && using_std_allocator) {
// Move elements directly in memory
const auto n_erase = last - first;
const auto n_after_erase = cend() - last;
if (n_erase >= n_after_erase) {
std::memcpy(
(void *) first.base(),
(void *) last.base(),
(cend() - last) * sizeof(T));
n_after_erase * sizeof(value_type));
} else {
std::memmove(
(void *) first.base(),
(void *) last.base(),
(cend() - last) * sizeof(T));
n_after_erase * sizeof(value_type));
}
} else {
// Move elements in memory
std::move(unconst(last), end(), unconst(first));
std::move(last, cend(), unconst(first));

// Destruct elements that were moved from and no longer in-use
// N.B. Only do this for non-relocatable types, otherwise you'd
// be running the destructor on exact byte copies of in-use
// elements, and you might free their internal buffers (oh no!).
if constexpr (!std::is_trivially_destructible_v<value_type>) {
std::for_each_n(
crbegin(),
n_erase,
[](value_type const &ele) { ele.~value_type(); });
}
}

// Directly set internal size. Elements are already destroyed.
set_internal_size(size() - std::distance(first, last));
return unconst(first);
Expand Down Expand Up @@ -1263,13 +1262,10 @@ namespace small {
if (new_size <= capacity()) {
return;
}
make_size_internal<std::false_type>(
new_size,
[](void *) {
make_size_internal<std::false_type>(new_size, [](void *) {
detail::throw_exception<std::logic_error>(
"Should not emplace when changing size");
},
0);
}, 0);
}

/// \brief Change the size and emplace the elements as we go
Expand Down Expand Up @@ -1530,7 +1526,8 @@ namespace small {
--out;
--in;
if constexpr (
is_relocatable_v<value_type> && using_std_allocator) {
is_relocatable_v<value_type> && using_std_allocator)
{
byte_copy(out, in, sizeof(value_type));
} else {
new (out) T(std::move(*in));
Expand All @@ -1542,7 +1539,8 @@ namespace small {
--out;
--in;
if constexpr (
is_relocatable_v<value_type> && using_std_allocator) {
is_relocatable_v<value_type> && using_std_allocator)
{
byte_copy(out, in, sizeof(value_type));
} else {
*out = std::move(*in);
Expand All @@ -1559,7 +1557,8 @@ namespace small {
while (out != first) {
--out;
if constexpr (
is_relocatable_v<value_type> && using_std_allocator) {
is_relocatable_v<value_type> && using_std_allocator)
{
new (out) T(create());
} else {
*out = create();
Expand Down Expand Up @@ -1808,16 +1807,14 @@ namespace small {

template <
class T,
size_t N
= (std::max)((24 * 2) / sizeof(T), std::size_t(5)),
size_t N = (std::max)((24 * 2) / sizeof(T), std::size_t(5)),
class Allocator = std::allocator<T>,
class SizeType = size_t>
using max_size_vector = vector<T, N, Allocator, std::false_type, SizeType>;

template <
class T,
size_t N
= (std::max)((24 * 2) / sizeof(T), std::size_t(5)),
size_t N = (std::max)((24 * 2) / sizeof(T), std::size_t(5)),
class Allocator = std::allocator<T>,
class SizeType = size_t>
using inline_vector = vector<T, N, Allocator, std::true_type, SizeType>;
Expand Down
1 change: 1 addition & 0 deletions test/unit/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ add_small_test(ptr_wrapper small::small)
add_small_test(pod_small_vector small::small)
add_small_test(string_small_vector small::small)
add_small_test(custom_small_vector small::small)
add_small_test(shared_ptr_small_vector small::small)

add_small_test(unicode_functions small::small)
add_small_test(small_string_make small::small)
Expand Down
33 changes: 33 additions & 0 deletions test/unit/shared_ptr_small_vector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//
// Copyright (c) 2022 Yat Ho ([email protected])
//
// Distributed under the Boost Software License, Version 1.0.
// https://www.boost.org/LICENSE_1_0.txt
//

// C++
#include <memory>

// External
#include <catch2/catch.hpp>

// Small
#include <small/vector.hpp>

TEST_CASE("Shared Ptr Vector") {
using namespace small;

STATIC_REQUIRE(!is_relocatable_v<std::shared_ptr<int>>);

SECTION("Erase in middle") {
vector<std::shared_ptr<int>> a;

for (int i = 0; i < 2; ++i) {
a.emplace_back(std::make_shared<int>(i));
}

a.erase(a.begin());

REQUIRE(*a[0] == 1);
}
}
Loading