hpx::is_trivially_relocatable trait implementation

[isidorostsa]

This PR deals with the implementation of the is_trivially_relocatable trait. It offers the user an interface to declare their custom class as trivially_relocatable using the macro:

HPX_DECLARE_TRIVIALLY_RELOCATABLE(<class name>)

This assumes that trivially_copyable types are also trivially relocatable, following Folly's convention: https://github.com/facebook/folly/blob/ec297b748575e8ab86333899295715e6e85f909d/folly/Traits.h#L542

[StellarBot]

Can one of the admins verify this patch?

[Quuxplusone]

Here I recommend

struct NotTriviallyCopyable1 {
    NotTriviallyCopyable1();
    NotTriviallyCopyable1(const NotTriviallyCopyable1&);
    NotTriviallyCopyable1& operator=(const NotTriviallyCopyable1&) = default;
    ~NotTriviallyCopyable1() = default;
};

struct NotTriviallyCopyable2 {
    NotTriviallyCopyable2();
    NotTriviallyCopyable2(const NotTriviallyCopyable2&) = default;
    NotTriviallyCopyable2& operator=(const NotTriviallyCopyable2&);
    ~NotTriviallyCopyable2() = default;
};

struct NotTriviallyCopyable3 {
    NotTriviallyCopyable3();
    NotTriviallyCopyable3(const NotTriviallyCopyable3&) = default;
    NotTriviallyCopyable3& operator=(const NotTriviallyCopyable3&) = default;
    ~NotTriviallyCopyable3();
};

None of these should be trivially relocatable according to P1144. NotTriviallyCopyable2 is controversial but correct: If you ever want to optimize vector::erase or swap_ranges, you can't let trivially relocatable types do anything weird in their assignment operators either.

[isidorostsa]

I see, what could a trivially relocatable type do in the assignment operator to break swap?

[Quuxplusone]

Literally anything other than "assign the value of the right-hand side to the left-hand side." I'm actually planning a blog post for this week or next, on all the things that potentially go wrong when we treat std::pmr::string as trivially relocatable.

struct Widget {
    std::pmr::string ps;
};

std::vector<Widget> w;
w.push_back(Widget{ std::pmr::string("hello"), &mr1) });
w.push_back(Widget{ std::pmr::string("world"), &mr2) });
w.erase(w.begin()); // ideally should destroy w[0] and relocate downward, right?
assert(w[0].ps.get_allocator().resource() == &mr1); // but today this is required to be true

auto v1 = std::pmr::vector<std::pmr::string>({"hello", "hello"}, &mr1);
auto v2 = std::pmr::vector<std::pmr::string>({"world", "world"}, &mr2);
std::swap_ranges(v1.begin(), v1.end(), v2.begin()); // ideally should swap the bytes, right?
assert(v1[0].get_allocator().resource() == &mr1); // but today this is required to be true

There are several theories of what to do here:

• Don't call std::pmr::string trivially relocatable at all. (This is the default, implied by the current wording of P1144R8.)
• Call std::pmr::string trivially relocatable, and put a precondition on all standard library facilities that says you're not allowed to pass them objects from different arenas; if you do, that's library UB. (This is my own preferred theory.)
• Call std::pmr::string trivially relocatable, and put a precondition on some standard library facilities that says you're not allowed to pass them objects from different arenas; if you do, that's library UB. For example, let's say vector<Widget>::erase gets to assume that all the Widgets in the vector have the same arena, but std::swap<Widget&> does not, and thus simply cannot be optimized for trivially relocatable types. (This is the Bloomberg/P2786R1 theory, as of last week, although it might change in the future.)

[Quuxplusone]

Here I recommend

struct ExplicitlyTriviallyRelocatable1 {
    ExplicitlyTriviallyRelocatable1();
    ExplicitlyTriviallyRelocatable1(const ExplicitlyTriviallyRelocatable1&);
    ExplicitlyTriviallyRelocatable1& operator=(const ExplicitlyTriviallyRelocatable1&);
    ~ExplicitlyTriviallyRelocatable1();
};
HPX_DECLARE_TRIVIALLY_RELOCATABLE(ExplicitlyTriviallyRelocatable1);

struct ExplicitlyTriviallyRelocatable2 {
    ExplicitlyTriviallyRelocatable2();
    ExplicitlyTriviallyRelocatable2(ExplicitlyTriviallyRelocatable2&&);
    ExplicitlyTriviallyRelocatable2& operator=(ExplicitlyTriviallyRelocatable2&&);
    ~ExplicitlyTriviallyRelocatable2();
};
HPX_DECLARE_TRIVIALLY_RELOCATABLE(ExplicitlyTriviallyRelocatable2);

struct DerivedFromExplicitlyTriviallyRelocatable : ExplicitlyTriviallyRelocatable1 {
    ~DerivedFromExplicitlyTriviallyRelocatable();
};

static_assert(hpx::is_trivially_relocatable_v<ExplicitlyTriviallyRelocatable1>);
static_assert(hpx::is_trivially_relocatable_v<ExplicitlyTriviallyRelocatable2>);
static_assert(!hpx::is_trivially_relocatable_v<DerivedFromExplicitlyTriviallyRelocatable>);

[isidorostsa]

From my understanding, the primary difference between 1 and 2 is that 1 is only copy assignable/constructible while 2 is only move assignable/constructible, with non-trivial functions for everything, making them both not trivially copyable. Does each structure represent a specific edge case scenario that the tests need to cover? I'm asking this to better understand the principles guiding the selection process for future test cases.

[Quuxplusone]

1 is both copyable and movable; it's a classic C++98 "Rule of Three" type, which you'll probably see a lot of in real code. The fact that when you move it you get a call to the copy constructor (not a separate move constructor) doesn't really matter to me. The point is just that it's not Rule-of-Zero and it isn't move-only.
2 is move-only, exactly isomorphic to unique_ptr.

The point of DerivedFromExplicitlyTriviallyRelocatable is that back when I was first thinking about "The Best Type Traits C++ Doesn't Have," I considered the idea of opting into things like trivial relocatability and trivial comparability by using a member typedef:

struct S {
    using is_trivially_relocatable = void;
};

(The STL uses this technique for is_transparent.) The downside to this is that then anything derived from S inherits that member typedef — i.e., anything derived from a trivially relocatable type ends up being marked as trivially relocatable — which obviously isn't going to work.

Now, neither P1144 nor HPX are actually doing anything at all with member typedefs, so we don't expect anything at all to go wrong with DerivedFromExplicitlyTriviallyRelocatable. But I figured it couldn't hurt to add the test case for it anyway.

[isidorostsa]

I see, both the tests and the rationale behind not using typedefs make sense, thank you :)

[hkaiser]

@Quuxplusone thanks for your comments! @isidorostsa if you apply the suggested changes, please stick with our naming conventions, though.

[isidorostsa]

@Quuxplusone thanks for your comments! @isidorostsa if you apply the suggested changes, please stick with our naming conventions, though.

Thanks for bringing that up, I looked and found that we require snake_case C++ STL style, is there something else I should keep in mind?

[hkaiser]

@Quuxplusone thanks for your comments! @isidorostsa if you apply the suggested changes, please stick with our naming conventions, though.

Thanks for bringing that up, I looked and found that we require snake_case C++ STL style, is there something else I should keep in mind?

Please see here for the whole story.

[isidorostsa]

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

[Quuxplusone]

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.

[isidorostsa]

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

[Quuxplusone]

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.

[isidorostsa]

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

[Quuxplusone]

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.

[isidorostsa]

| 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)?

[Quuxplusone]

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.)

[isidorostsa]

@hkaiser

• I applied the styling changes and east const to the tests of is_relocatable as well
• Still need to move this whole implementation over to type_support, could you please verify that as a sanity check?

[hkaiser]

Let's have these low-level facilities in core/type_support after all (as discussed). Otherwise we will not be able to use this for the implementation of our own data types (small_vector, etc.).

[hkaiser]

Do we have to disable this specialization if T was marked as trivially relocatable (by specializing is_trivially_relocatable<T>) where T is also trivially copyable?

Alternatively, we could change the macro above to:

#define HPX_DECLARE_TRIVIALLY_RELOCATABLE(T)                                   \
    namespace hpx {                                                            \
        template <>                                                            \
        struct is_trivially_relocatable<T,                                     \
            std::enable_if_t<!std::is_trivially_copyable_v<T>>>                \
          : std::true_type                                                     \
        {                                                                      \
        };                                                                     \
    }

[Quuxplusone]

Alternatively, just change the base case to

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

[isidorostsa]

Thank you that's very elegant, I will apply this. Would

std::enable_if_t<!std::is_trivially_relocatable_v>> ...

work as a more general solution? It does look kind of suspicious...

[Quuxplusone]

No, you don't want to define is_trivially_relocatable in terms of itself. ;)
Notice that P1144 can't help with this part of the HPX design, because P1144 proposes that the core language should just know which types are trivially relocatable — P1144's raison d'etre is specifically to avoid this kind of opt-in macro. (But HPX needs this part because P1144's core-language changes aren't in the language yet.)

@isidorostsa: You should definitely take a look at what BSL and Folly are doing with their opt-in macros, and try to evaluate what advantages and disadvantages each approach might have. I think what you're doing with just letting the user specialize hpx::is_trivially_relocatable<T, enable_if_t<...>> is fine, but I don't think it is isomorphic to either BSL's or Folly's approach, so it would be good to evaluate, justify, and document.

• bslmf::IsBitwiseMoveable
• folly::IsRelocatable

[isidorostsa]

Thanks for this note, having examined bslmf and folly we decided to avoid using a nested trait, due to it being invasive to existing code to work. We did implement more generic macros to compensate for the lack of template and conditional support by the previous macro.

new macros: 7139cb1

[hkaiser]

LGTM, thanks!

[hkaiser]

bors merge

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• Bors