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.

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

Constexpr-ify inplace_vector #21

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
99 changes: 65 additions & 34 deletions include/beman/inplace_vector/inplace_vector.hpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include <algorithm>
#include <array>
#include <cassert>
#include <cstddef>
#include <exception>
#include <iterator>
#include <limits>
Expand Down Expand Up @@ -54,36 +56,68 @@ using inplace_vector_internal_size_type =
If<Capacity <= std::numeric_limits<uint32_t>::max(), uint32_t,
uint64_t>>>;

// array based storage is used so that we can satisfy constexpr requirement
//
// Selecting this storage type implies: std::is_trivial_v<T> or Capacity = 0
template <typename T, std::size_t Capacity>
struct inplace_vector_type_based_storage {
using array_type = If<!std::is_const_v<T>, std::array<T, Capacity>,
std::array<std::remove_const_t<T>, Capacity>>;
array_type elems;

constexpr T *begin() { return elems.data(); }
constexpr const T *begin() const { return elems.data(); }
};

// byte based storage is used for non-constexpr environment. Objects of type T
// are only constructed when needed and they don't even have to be default
// constructible.
//
// Selecting this storage type implies: !std::is_trivial_v<T> and Capacity != 0
template <typename T, std::size_t Capacity>
struct inplace_vector_bytes_based_storage {
alignas(T) std::array<std::byte, Capacity * sizeof(T)> elems;

T *begin() { return std::launder(reinterpret_cast<T *>(elems.data())); }
const T *begin() const {
return std::launder(reinterpret_cast<const T *>(elems.data()));
}
};

// Base class for inplace_vector
template <typename T, std::size_t Capacity>
struct inplace_vector_destruct_base {
using size_type = std::size_t;
using internal_size_type = inplace_vector_internal_size_type<Capacity>;
using internal_storage_type =
std::conditional_t<std::is_trivial_v<T> or Capacity == 0,
inplace_vector_type_based_storage<T, Capacity>,
inplace_vector_bytes_based_storage<T, Capacity>>;

alignas(T) unsigned char elems[Capacity * sizeof(T)] = {};
internal_storage_type elems;
internal_size_type size_{0};

// [containers.sequences.inplace.vector.cons], construct/copy/destroy
constexpr inplace_vector_destruct_base() = default;

inplace_vector_destruct_base(
constexpr inplace_vector_destruct_base(
const inplace_vector_destruct_base
&other) noexcept(std::is_nothrow_copy_constructible_v<T>)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.

Copy link
Member Author

@wusatosi wusatosi Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay I will mark a TODO here. There's a lot of non-conformity in this implementation.

I think we should defer this to another PR, there's too much to be updated.

: elems(), size_(other.size_) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change, but I need to make some remark on all constructors in inplace_vector_destruct_base.
IMO, it would be O.K. to address this in a later PR.

They all value-initialise elems, i.e. the bytes in bytes_based_Storage become 0 and all elements in type_based_storage are value initialised. I tried to argue above why I don't like this, they should be initialised only when a vector element is really constructed in the place.

Besides (superfluously) initialising elems, they all do the same thing, they set size_ to some value. That is, they avoid doing what a copy c'tor, move c'tor etc. normally does and instead manipulate size_only. This is redundant. Instead, a single constructor taking a size would do. This could be called by the all the constructors in the deriving class. size_could probably be made private.

Copy link
Member Author

@wusatosi wusatosi Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inplace_vector_destruct_base needs more work on trivially destructible requirement as well, needs to switch between a trivially destructible base and a non-trivial destructor against T.

Unfortunately intention in this repo is to support C++17, so we cannot use requires. This have to be done with template.


inplace_vector_destruct_base(
constexpr inplace_vector_destruct_base(
const inplace_vector_destruct_base
&&other) noexcept(std::is_nothrow_move_constructible_v<T>)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change, but to me this is unconditionally noexcept, since no T's are moved at all.

: elems(), size_(other.size()) {}
: elems(), size_(other.size_) {}

inplace_vector_destruct_base &
constexpr inplace_vector_destruct_base &
operator=(const inplace_vector_destruct_base &other) noexcept(
std::is_nothrow_copy_constructible_v<T> &&
std::is_nothrow_copy_assignable_v<T>) {
Comment on lines 115 to 116
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.

size_ = other.size_;
}

inplace_vector_destruct_base &
constexpr inplace_vector_destruct_base &
operator=(const inplace_vector_destruct_base &&other) noexcept(
std::is_nothrow_move_constructible_v<T> &&
std::is_nothrow_move_assignable_v<T>) {
Comment on lines 122 to 123
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.

Expand All @@ -108,19 +142,18 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {

// [containers.sequences.inplace.vector.cons], construct/copy/destroy
constexpr inplace_vector_base() = default;
inplace_vector_base(const inplace_vector_base &other) noexcept(
constexpr inplace_vector_base(const inplace_vector_base &other) noexcept(
std::is_nothrow_copy_constructible_v<T>)
: inplace_vector_destruct_base<T, Capacity>(other.size) {
: inplace_vector_destruct_base<T, Capacity>(other.size_) {
std::copy(other.begin(), other.end(), begin());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change, but needs to be an uninitialised_copy, in particular for the byte based storage case.
It happens work in the type based storage case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is uninitialised_copy needed here?

}
inplace_vector_base(inplace_vector_base &&other) noexcept(
constexpr inplace_vector_base(inplace_vector_base &&other) noexcept(
Capacity == 0 || std::is_nothrow_move_constructible_v<T>)
: inplace_vector_destruct_base<T, Capacity>(other.size_) {
std::copy(other.begin(), other.end(), begin());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change, but must be uninitalized_move, see above.

std::destroy(other.begin(), other.end());
other.size_ = 0;
}
inplace_vector_base &operator=(const inplace_vector_base &other) noexcept(
constexpr inplace_vector_base &
operator=(const inplace_vector_base &other) noexcept(
std::is_nothrow_copy_constructible_v<T> &&
std::is_nothrow_copy_assignable_v<T>) {
const auto diff = static_cast<std::ptrdiff_t>(other.size() - size());
Expand All @@ -141,7 +174,8 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {
return *this;
}

inplace_vector_base &operator=(inplace_vector_base &&other) noexcept(
constexpr inplace_vector_base &
operator=(inplace_vector_base &&other) noexcept(
Capacity == 0 || (std::is_nothrow_move_constructible_v<T> &&
std::is_nothrow_move_assignable_v<T>)) {
const auto diff = static_cast<std::ptrdiff_t>(other.size() - size());
Expand All @@ -151,13 +185,10 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {
std::destroy(new_end, end());
// other size is grater than size
} else {
std::move(other, other.begin(), other.begin() + size(), begin());
std::move(other.begin(), other.begin() + size(), begin());
std::move(other.begin() + size(), other.end(), end());
}
this->size_ = other.size();
std::destroy(other.begin(), other.end());
// reset size to zero
other.change_size(-static_cast<std::ptrdiff_t>(other.size()));
return *this;
}
constexpr inplace_vector_base(const size_type size)
Expand All @@ -173,28 +204,27 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {
constexpr bool empty() const noexcept { return this->size_ == 0; }

// [containers.sequences.inplace.vector.data], data access
constexpr T *data() noexcept {
return std::launder(reinterpret_cast<T *>(this->elems));
}
constexpr const T *data() const noexcept {
return std::launder(reinterpret_cast<const T *>(this->elems));
}
constexpr T *data() noexcept { return this->elems.begin(); }
constexpr const T *data() const noexcept { return this->elems.begin(); }

// [containers.sequences.inplace.vector.iterators] iterators
iterator begin() noexcept {
return std::launder(reinterpret_cast<T *>(this->elems));
constexpr iterator begin() noexcept {
if constexpr (Capacity == 0)
return nullptr;
else
return this->elems.begin();
}
const_iterator begin() const noexcept {
return std::launder(reinterpret_cast<const T *>(this->elems));
constexpr const_iterator begin() const noexcept {
if constexpr (Capacity == 0)
return nullptr;
else
return this->elems.begin();
}

iterator end() noexcept {
return std::launder(reinterpret_cast<T *>(this->elems) + this->size());
}
constexpr iterator end() noexcept { return begin() + this->size(); }

const_iterator end() const noexcept {
return std::launder(reinterpret_cast<const T *>(this->elems) +
this->size());
constexpr const_iterator end() const noexcept {
return begin() + this->size();
}

// [containers.sequences.inplace.vector.modifiers], modifiers
Expand Down Expand Up @@ -576,7 +606,8 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
#else
// Note: placement-new may not be constexpr friendly
// Avoiding placement-new may allow inplace_vector to be constexpr friendly
auto final = ::new (end()) T(std::forward<Args>(args)...);
// cast to void* here to adapt to a const T
auto final = ::new ((void *)end()) T(std::forward<Args>(args)...);
#endif
this->change_size(1);
return *final;
Expand Down
15 changes: 15 additions & 0 deletions tests/beman/inplace_vector/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,18 @@ add_executable(beman.inplace_vector.test inplace_vector.test.cpp)
target_link_libraries(beman.inplace_vector.test PRIVATE beman.inplace_vector)

add_test(NAME beman.inplace_vector.test COMMAND beman.inplace_vector.test)

if(CMAKE_CXX_STANDARD GREATER_EQUAL 20)
# constexpr test
add_executable(beman.inplace_vector.constexpr_test constexpr.test.cpp)
target_link_libraries(
beman.inplace_vector.constexpr_test
PRIVATE beman.inplace_vector
)
add_test(
NAME beman.inplace_vector.constexpr_test
COMMAND beman.inplace_vector.constexpr_test
)
else()
message(WARNING "C++20 or later is not enabled, skipping constexpr test.")
endif()
Loading