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

Lower lowest supported version to C++17. #22

Merged
merged 6 commits into from
Nov 19, 2024
Merged

Lower lowest supported version to C++17. #22

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
09b9676
remove c++ 20 explicit feature
wusatosi Nov 18, 2024
7d2676d
update CI to test C++17
wusatosi Nov 18, 2024
0673137
update readme to reflect C++17 support
wusatosi Nov 18, 2024
1eb14d7
selectivly use construct_at
wusatosi Nov 18, 2024
ceb4932
better comment
wusatosi Nov 18, 2024
8fa4168
fix comment
wusatosi Nov 19, 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
4 changes: 3 additions & 1 deletion .github/workflows/ci_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ jobs:
c: gcc
- cpp: clang++
c: clang
cpp_version: [20, 23, 26]
cpp_version: [17, 20, 23, 26]
cmake_args:
- description: "Default"
args: ""
Expand All @@ -46,6 +46,8 @@ jobs:
- description: "ASan"
args: "-DCMAKE_CXX_FLAGS=-fsanitize=address -fsanitize=undefined"
include:
# Needs C++ 20 as C++17 selectivly disables ranges and concepts
# related functionalities
- platform: ubuntu-latest
compiler:
cpp: g++
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ which dynamic memory allocations are undesired.

### Compiler support

Building this repository requires **C++20** or later.
Building this repository requires **C++17** or later.

### Dependencies

Expand Down
75 changes: 54 additions & 21 deletions include/beman/inplace_vector/inplace_vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,29 @@

#include <algorithm>
#include <cassert>
#include <compare>
#include <concepts>
#include <exception>
#include <iterator>
#include <limits>
#include <memory>
#include <new>
#include <ranges>
#include <stdexcept>
#include <type_traits>
#include <utility>

#ifdef __cpp_concepts
#include <concepts>
#endif

#ifdef __cpp_lib_containers_ranges
#include <ranges>
#endif

#ifdef __cpp_lib_three_way_comparison
#include <compare>
#endif

namespace beman::inplace_vector {
#ifdef __cpp_concepts
namespace detail {
// Exposition-only container-compatible-range
template <typename T>
Expand All @@ -29,6 +40,7 @@ concept container_compatible_range_impl = requires(T &&t) {
template <typename T>
constexpr bool container_compatible_range =
detail::container_compatible_range_impl<T>;
#endif

template <bool Condition, typename TrueType, typename FalseType>
using If = typename std::conditional<Condition, TrueType, FalseType>::type;
Expand Down Expand Up @@ -99,12 +111,12 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {
inplace_vector_base(const inplace_vector_base &other) noexcept(
std::is_nothrow_copy_constructible_v<T>)
: inplace_vector_destruct_base<T, Capacity>(other.size) {
std::ranges::copy(other.begin(), other.end(), begin());
std::copy(other.begin(), other.end(), begin());
DeveloperPaul123 marked this conversation as resolved.
Show resolved Hide resolved
}
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::ranges::copy(other.begin(), other.end(), begin());
std::copy(other.begin(), other.end(), begin());
std::destroy(other.begin(), other.end());
other.size = 0;
}
Expand All @@ -114,17 +126,16 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {
const auto diff = static_cast<std::ptrdiff_t>(other.size() - size());
// other.size is less than size
if (diff < 0) {
const auto new_end =
std::ranges::copy(other.begin(), other.end(), begin());
const auto new_end = std::copy(other.begin(), other.end(), begin());
// destroy unnecessary memory
std::destroy(new_end, end());
}
// other.size is greater than size
else {
// copy other vector into the current vector until it runs ouf of size
std::ranges::copy(other.begin(), other.begin() + size(), begin());
std::copy(other.begin(), other.begin() + size(), begin());
// copy the other half after the end of the current vector
std::ranges::copy(other.begin() + size(), other.end(), end());
std::copy(other.begin() + size(), other.end(), end());
}
this->size_ = other.size();
return *this;
Expand Down Expand Up @@ -200,7 +211,7 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {
template <typename Iter>
static constexpr void uninitialized_copy(Iter first, Iter last,
iterator dest) noexcept {
std::ranges::copy(first, last, dest);
std::copy(first, last, dest);
}

template <typename Iter>
Expand Down Expand Up @@ -244,13 +255,15 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
base::uninitialized_fill(this->begin(), this->end(), value);
}

#ifdef __cpp_concepts
template <class InputIterator>
requires std::input_iterator<T>
constexpr inplace_vector(InputIterator first, InputIterator last) : base() {
for (; first != last; ++first) {
emplace_back(*first);
}
}

template <class InputIterator>
requires std::forward_iterator<T>
constexpr inplace_vector(InputIterator first, InputIterator last)
Expand All @@ -259,15 +272,22 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
base::uninitialized_copy(first, last, this->begin());
}
}
#ifdef __cpp_lib_containers_ranges
#else
template <class Itr> constexpr inplace_vector(Itr first, Itr last) : base() {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth constraining Itr here using old school SFINAE?

Copy link
Member Author

@wusatosi wusatosi Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good question...

Copy link
Member Author

Choose a reason for hiding this comment

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

Egh! I do hate writing enable_if and template magics.

I think we should leave this as is, we can come back and write constraints as cherry-on-top.

There's other things (tests and constexpr) I want to focus on.

for (; first != last; ++first) {
emplace_back(*first);
}
}
#endif

#if defined(__cpp_lib_containers_ranges) && defined(__cpp_concepts)
template <typename R>
requires container_compatible_range<R>
constexpr inplace_vector(std::from_range_t, R &&rg) {
for (auto &&value : rg) {
emplace_back(std::forward<decltype(value)>(value));
}
}
#else
#endif
constexpr inplace_vector(std::initializer_list<T> il) : base(il.size()) {
if (il.size() != 0) {
Expand All @@ -282,14 +302,13 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
// The current size is greater
if (diff < 0) {
// if other.size is less than just copy normally
const iterator new_end =
std::ranges::copy(il.begin(), il.end(), this->begin());
const iterator new_end = std::copy(il.begin(), il.end(), this->begin());
// destroy the wasted memory
std::destroy(new_end, this->end());
// The other size is greater than size
} else {
// copy other vector into the current vector until it runs ouf of size
std::ranges::copy(il.begin(), il.begin() + this->size(), this->begin());
std::copy(il.begin(), il.begin() + this->size(), this->begin());
// copy the other half after the end of the current vector
base::uninitialized_copy(il.begin() + this->size(), il.end(),
this->end());
Expand Down Expand Up @@ -318,6 +337,7 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
}
}; // freestanding-deleted

#if defined(__cpp_lib_containers_ranges) and defined(__cpp_concepts)
template <typename R>
requires container_compatible_range<R>
constexpr void assign_range(R &&rg) {
Expand All @@ -340,6 +360,8 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
emplace_back(*first);
}
}; // freestanding-deleted
#endif

constexpr void assign(size_type n, const T &u) {
if (Capacity == 0) {
assert(size() == 0 &&
Expand All @@ -366,13 +388,12 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
// other size is less than size
if (diff < 0) {
// if other.size is less than just copy normally
const iterator new_end =
std::ranges::copy(il.begin(), il.end(), this->begin());
const iterator new_end = std::copy(il.begin(), il.end(), this->begin());
std::destroy(new_end, this->end);
// other.size is greater than size
} else {
// copy other vector into the current vector until it runs ouf of size
std::ranges::copy(il.begin(), il.begin() + this->size(), this->begin());
std::copy(il.begin(), il.begin() + this->size(), this->begin());
// copy the other half after the end of the current vector
base::uninitialized_copy(il.begin() + this->size(), il.end(),
this->end());
Expand Down Expand Up @@ -491,6 +512,8 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
}
return this->unchecked_emplace_back(std::move(x));
}; // freestanding-deleted

#if defined(__cpp_lib_containers_ranges) and defined(__cpp_concepts)
template <typename R>
requires container_compatible_range<T>
constexpr void append_range(R &&rg) {
Expand All @@ -500,6 +523,8 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
emplace_back(*first);
}
}; // freestanding-deleted
#endif

constexpr void pop_back() {
if (!empty()) {
const auto end = this->end();
Expand Down Expand Up @@ -546,7 +571,10 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {

template <class... Args>
constexpr reference unchecked_emplace_back(Args &&...args) {
auto final = std::construct_at(end(), std::forward<Args>(args)...);
// TODO: what is the feature flag for std::construct_at ??
// auto final = std::construct_at(end(), std::forward<Args>(args)...);
// This is just construct_at expanded out.
auto final = ::new (end()) T(std::forward<Args>(args)...);
Copy link
Member Author

Choose a reason for hiding this comment

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

How do I selectively use construct_at instead of placement new?

construct_at doesn't have a feature-test macro.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DeveloperPaul123 do you know how to approach this?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it could be __cpp_lib_constexpr_dynamic_alloc or __cpp_constexpr_dynamic_alloc as construct_at was added in P0784R7

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 thanks!

I will add a patch later.

I have a question for this, is there a constexpr construct_at for C++17? Placement new doesn't seem to be constexpr friendly.

Copy link
Member

Choose a reason for hiding this comment

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

No problem!

And no... at least not that I'm aware of off the top of my head. I would have to do some digging to find out.

Copy link
Member Author

@wusatosi wusatosi Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Alright I guess constexpr stays at C++20 then.

this->change_size(1);
return *final;
};
Expand Down Expand Up @@ -644,13 +672,14 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
InputIterator imiddle = std::next(first, to_copy);
base::uninitialized_copy(imiddle, last, end);
base::uninitialized_move(pos, end, middle);
std::ranges::copy(first, imiddle, pos);
std::copy(first, imiddle, pos);
} else {
base::uninitialized_move(end - count, end, end);
std::move_backward(pos, end - count, end);
std::ranges::copy(first, last, pos);
std::copy(first, last, pos);
}
} // freestanding-deleted
#if defined(__cpp_lib_containers_ranges) and defined(__cpp_concepts)
template <typename R>
requires container_compatible_range<R>
constexpr iterator insert_range(const_iterator position, R &&rg) {
Expand All @@ -665,6 +694,7 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
std::rotate(pos, old_end, this->end());
return pos;
} // freestanding-deleted
#endif
constexpr iterator insert(const_iterator position,
std::initializer_list<T> il) {
const iterator pos = position;
Expand Down Expand Up @@ -744,10 +774,13 @@ class inplace_vector : public inplace_vector_base<T, Capacity> {
return std::equal(x.begin(), x.end(), y.begin(), y.end());
}

#ifdef __cpp_lib_three_way_comparison
constexpr friend std::compare_three_way_result<T>
operator<=>(const inplace_vector &x, const inplace_vector &y) {
return std::lexicographical_compare(x.begin(), x.end(), y.begin(), y.end());
};
#endif

constexpr friend void swap(inplace_vector &x, inplace_vector &y) noexcept(
Capacity == 0 || (std::is_nothrow_swappable_v<T> &&
std::is_nothrow_move_constructible_v<T>)) {
Expand Down