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

hpx::is_trivially_relocatable trait implementation #6264

Merged
merged 28 commits into from
Jul 10, 2023
Merged

hpx::is_trivially_relocatable trait implementation #6264

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
1f88873
move_constructible impies destructible
isidorostsa May 27, 2023
a179c52
Trivially relocatable as a subset of trivially_copyable
isidorostsa May 28, 2023
c435377
trivially_relocatable unit tests
isidorostsa May 28, 2023
9f2c184
add trait and test to cmake
isidorostsa May 28, 2023
1db200e
typo in cmake
isidorostsa May 28, 2023
c8ff8dc
clang-formated
isidorostsa May 28, 2023
0603265
Update libs/core/algorithms/include/hpx/algorithms/traits/is_triviall…
isidorostsa May 28, 2023
0b46dd6
trivially copyable covers poitners, iterators are removed
isidorostsa May 29, 2023
f63342b
Testing triviallyCopyable and not cases
isidorostsa May 29, 2023
442972a
clang-format
isidorostsa May 29, 2023
76dcad2
newline in the end
isidorostsa May 29, 2023
4221069
use existing `std::bool_constant`
isidorostsa May 29, 2023
1b39238
remove typename
isidorostsa May 30, 2023
689a1fc
CamelCase to snake_case
isidorostsa May 30, 2023
c2acd4f
CamelCase to snake_case
isidorostsa May 30, 2023
6caee74
Added recomended tests
isidorostsa May 30, 2023
863e5a6
clang-format
isidorostsa May 30, 2023
5e7bb7a
non_trivial_but_trivially_copyable correction
isidorostsa Jun 1, 2023
9e1d723
misspelled "primitive"
isidorostsa Jun 2, 2023
89cf296
adapt old tests to hpx styleguide
isidorostsa Jun 15, 2023
24a1890
use East const convention
isidorostsa Jun 15, 2023
e6d0384
Update libs/core/algorithms/include/hpx/algorithms/traits/is_relocata…
isidorostsa Jun 19, 2023
8e39def
relocating relocation
isidorostsa Jun 19, 2023
170b8df
rename tests
isidorostsa Jun 19, 2023
c7d3639
change include inside pointer_category
isidorostsa Jun 19, 2023
347cacb
clang-format
isidorostsa Jun 19, 2023
7139cb1
Support templated classes (conditionally) (#4)
isidorostsa Jul 3, 2023
3b626c4
fix spelling error
isidorostsa Jul 9, 2023
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
1 change: 1 addition & 0 deletions libs/core/algorithms/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
set(algorithms_headers
hpx/algorithms/traits/is_pair.hpp
hpx/algorithms/traits/is_relocatable.hpp
hpx/algorithms/traits/is_trivially_relocatable.hpp
hpx/algorithms/traits/is_value_proxy.hpp
hpx/algorithms/traits/pointer_category.hpp
hpx/algorithms/traits/projected.hpp
Expand Down
4 changes: 1 addition & 3 deletions libs/core/algorithms/include/hpx/algorithms/traits/is_relocatable.hpp
View file
Open in desktop
isidorostsa marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@
namespace hpx {

template <typename T>
struct is_relocatable
struct is_relocatable : std::bool_constant<std::is_move_constructible_v<T>>
isidorostsa marked this conversation as resolved.
Show resolved Hide resolved
{
static constexpr bool value =
std::is_move_constructible_v<T> && std::is_destructible_v<T>;
};

template <typename T>
Expand Down
38 changes: 38 additions & 0 deletions libs/core/algorithms/include/hpx/algorithms/traits/is_trivially_relocatable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright (c) 2023 Isidoros Tsaousis-Seiras
//
// SPDX-License-Identifier: BSL-1.0
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#pragma once

#include <hpx/iterator_support/traits/is_iterator.hpp>
#include <type_traits>

// Macro to specialize template for given type
#define HPX_DECLARE_TRIVIALLY_RELOCATABLE(T) \
namespace hpx { \
template <> \
struct is_trivially_relocatable<T> : std::true_type \
{ \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

last minute thought: adding static_assert(is_relocatable) ; as it is impossible to be trivially_relocatable but not relocatable.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I thought as of P1144R6; but the current revision (P1144R8) adopts the viewpoint that "trivially relocatable" is like "trivially copyable." It implies, "I promise that hypothetically when you (copy|relocate|compare) this thing, you can do it trivially." It doesn't imply anything about whether it is actually okay to (copy|relocate|compare) the thing; that's up to the algorithm's author to check.

E.g.

struct A { int i; A(A&&) = delete; A& operator=(A&&) = delete; };
static_assert(std::is_trivially_copyable_v<A>);  // C++ today
static_assert(std::is_trivially_relocatable_v<A>);  // in P1144R8
A src[10];
A dst[10];
alignas(A) char rawdst[10*sizeof(A)];
std::copy(src, src+10, dst);  // invalid C++ today
std::uninitialized_move(src, src+10, (A*)rawdst);  // also invalid C++ today
std::uninitialized_relocate(src, src+10, (A*)rawdst);  // also invalid in P1144R8

...however, I see that I need to update my libc++ implementation to actually reject the latter. ;)

Anyway, I don't have a strong opinion on which way HPX should define their trait. If you want to make it false for non-relocatable types, that's not crazy; it won't explode or anything. But I did want to point out that P1144R8 chooses to do it differently, and why P1144R8 chooses to do it differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I am struggling to think of a case where using std::relocate on a trivially relocatable but not relocatable type would be correct. I think I must be missing something

Copy link
Contributor

Choose a reason for hiding this comment

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

I am struggling to think of a case where using [std::uninitialized_relocate] on a trivially relocatable but not relocatable type would be correct.

That's the point: it'd never be correct. And neither would it be correct to use std::copy on a trivially copyable but not copy-assignable type. That doesn't mean such a type is not trivially copyable (or not trivially relocatable); it just means that you can't use it with those algorithms because they require copy-assignability resp. relocatability.

The interface constraint on std::copy is that the type you pass in needs to be is_copy_assignable. Internally, the optimization into a memcpy kicks in when the type happens to be is_trivially_copyable.

The interface constraint on std::uninitialized_relocate is that the type you pass in needs to be is_relocatable. Internally, the optimization into a memcpy kicks in when the type happens to be is_trivially_relocatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh I see, so it's just a matter of where this check should be performed, R8 suggests it should not throw a compile error when you give a non-relocatable type this trait, but it should when we try to relocate it, right? And this is checked with a static assertion inside the relocation algorithms

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe you've got it now. Here's a concrete example with libc++: https://godbolt.org/z/Yr9MdncoT

struct S {
    const std::unique_ptr<int> m;
};

static_assert(std::is_trivially_relocatable_v<S>);
static_assert(!std::is_relocatable_v<S>);

void f(S *p) {
    std::relocate_at(p, p); // ill-formed
}

__memory/relocate_at.h:91:19: error: static assertion failed [...]

If you could relocate S, then you could do so trivially. But in fact you can't relocate S.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

| If you could relocate S, then you could do so trivially. But in fact you can't relocate S.

Just for a sanity check; arrays of trivially relocatable types T[] should be considered trivially relocatable, but not relocatable (as not move constructable)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for a sanity check; arrays of trivially relocatable types T[] should be considered trivially relocatable, but not relocatable (as not move constructable)?

Yes, that's exactly right. https://godbolt.org/z/9n4YGTjfd (P1144R8 has actually removed std::is_relocatable_v, but kept std::relocatable. My Godbolt implementation keeps both, at least for now, since they're trivial to implement.)

}; \
}

namespace hpx {

template <typename T, typename = void>
struct is_trivially_relocatable : std::false_type
{
};

// All trivially copyable types are trivially relocatable
template <typename T>
struct is_trivially_relocatable<T,
std::enable_if_t<std::is_trivially_copyable_v<T>>> : std::true_type
{
};

template <typename T>
inline constexpr bool is_trivially_relocatable_v =
is_trivially_relocatable<T>::value;
} // namespace hpx
10 changes: 8 additions & 2 deletions libs/core/algorithms/tests/unit/algorithms/util/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,14 @@
# Distributed under the Boost Software License, Version 1.0. (See accompanying
# file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

set(tests test_is_relocatable test_low_level test_merge_four test_merge_vector
test_nbits test_range
set(tests
test_is_relocatable
test_is_trivially_relocatable
test_low_level
test_merge_four
test_merge_vector
test_nbits
test_range
)

foreach(test ${tests})
Expand Down
155 changes: 155 additions & 0 deletions libs/core/algorithms/tests/unit/algorithms/util/test_is_trivially_relocatable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
// Copyright (c) 2023 Isidoros Tsaousis-Seiras
//
// SPDX-License-Identifier: BSL-1.0
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#include <hpx/algorithms/traits/is_trivially_relocatable.hpp>

#include <cassert>

// Trivially copyable types are trivially relocatable
struct empty
{
};

struct non_empty_but_trivial
{
int i;
};

struct non_trivial_but_trivially_copyable
{
explicit non_trivial_but_trivially_copyable(int);
int i;
};

struct move_only_but_trivially_copyable
{
int i;
explicit move_only_but_trivially_copyable(int);
move_only_but_trivially_copyable(
move_only_but_trivially_copyable&&) noexcept = default;
move_only_but_trivially_copyable& operator=(
move_only_but_trivially_copyable&&) noexcept = default;
~move_only_but_trivially_copyable() = default;
};

struct non_assignable_but_trivially_copyable
{
int i;
explicit non_assignable_but_trivially_copyable(int);
non_assignable_but_trivially_copyable(
non_assignable_but_trivially_copyable&&) = default;
non_assignable_but_trivially_copyable operator=(
non_assignable_but_trivially_copyable&&) = delete;
~non_assignable_but_trivially_copyable() = default;
};

static_assert(hpx::is_trivially_relocatable_v<empty>);
static_assert(hpx::is_trivially_relocatable_v<non_empty_but_trivial>);
static_assert(
hpx::is_trivially_relocatable_v<non_trivial_but_trivially_copyable>);
static_assert(
hpx::is_trivially_relocatable_v<move_only_but_trivially_copyable>);
static_assert(
hpx::is_trivially_relocatable_v<non_assignable_but_trivially_copyable>);

// Has non trivial copy constructor
struct not_trivially_copyable_1
{
not_trivially_copyable_1();
not_trivially_copyable_1(const not_trivially_copyable_1&);
not_trivially_copyable_1& operator=(
const not_trivially_copyable_1&) = default;
~not_trivially_copyable_1() = default;
};

// Has non trivial copy assignment
struct not_trivially_copyable_2
{
not_trivially_copyable_2();
not_trivially_copyable_2(const not_trivially_copyable_2&) = default;
not_trivially_copyable_2& operator=(const not_trivially_copyable_2&);
~not_trivially_copyable_2() = default;
};

// Has non trivial destructor
struct not_trivially_copyable_3
{
not_trivially_copyable_3();
not_trivially_copyable_3(const not_trivially_copyable_3&) = default;
not_trivially_copyable_3& operator=(
const not_trivially_copyable_3&) = default;
~not_trivially_copyable_3();
};

// Non trivially copyable types are not trivially relocatable
// Unless they are explicitly declared as such
static_assert(!hpx::is_trivially_relocatable_v<not_trivially_copyable_1>);
static_assert(!hpx::is_trivially_relocatable_v<not_trivially_copyable_2>);
static_assert(!hpx::is_trivially_relocatable_v<not_trivially_copyable_2>);

// Not trivially copyable
struct explicitly_trivially_relocatable_1
{
explicitly_trivially_relocatable_1() = default;
explicitly_trivially_relocatable_1(
const explicitly_trivially_relocatable_1&);
explicitly_trivially_relocatable_1& operator=(
const explicitly_trivially_relocatable_1&);
~explicitly_trivially_relocatable_1() = default;
};

// Has non trivial copy assignment
struct explicitly_trivially_relocatable_2
{
explicitly_trivially_relocatable_2() = default;
explicitly_trivially_relocatable_2(explicitly_trivially_relocatable_2&&);
explicitly_trivially_relocatable_2& operator=(
explicitly_trivially_relocatable_2&&);
~explicitly_trivially_relocatable_2() = default;
};

HPX_DECLARE_TRIVIALLY_RELOCATABLE(explicitly_trivially_relocatable_1);
HPX_DECLARE_TRIVIALLY_RELOCATABLE(explicitly_trivially_relocatable_2);

// Explicitly declared trivially relocatable types are trivially relocatable
static_assert(
hpx::is_trivially_relocatable_v<explicitly_trivially_relocatable_1>);
static_assert(
hpx::is_trivially_relocatable_v<explicitly_trivially_relocatable_2>);

// Trivial relocatability is not inherited
struct derived_from_explicitly_trivially_relocatable
: explicitly_trivially_relocatable_1
{
};

static_assert(!hpx::is_trivially_relocatable_v<
derived_from_explicitly_trivially_relocatable>);

// Polymorphic types are not trivially relocatable
struct polymorphic
{
virtual int f();
};
static_assert(!hpx::is_trivially_relocatable_v<polymorphic>);

// Primitive data types are trivially relocatable
static_assert(hpx::is_trivially_relocatable_v<int>,
"int should be Trivially Relocatable");
static_assert(hpx::is_trivially_relocatable_v<double>,
"double should be Trivially Relocatable");
static_assert(hpx::is_trivially_relocatable_v<char>,
"char should be Trivially Relocatable");
static_assert(hpx::is_trivially_relocatable_v<void*>,
"void* should be Trivially Relocatable");
static_assert(hpx::is_trivially_relocatable_v<int*>,
"int* should be Trivially Relocatable");
static_assert(hpx::is_trivially_relocatable_v<double*>,
"double* should be Trivially Relocatable");
static_assert(hpx::is_trivially_relocatable_v<char*>,
"char* should be Trivially Relocatable");

int main(int, char*[]) {}
isidorostsa marked this conversation as resolved.
Show resolved Hide resolved