Add test to see if indirect is trivially relocatable

[jbcoe]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (19bcd70) 100.00% compared to head (8e46c0d) 100.00%.
Report is 7 commits behind head on main.

❗ Current head 8e46c0d differs from pull request most recent head 312ec44. Consider uploading reports for the commit 312ec44 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #302   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          457       457           
=========================================
  Hits           457       457           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Quuxplusone]

"This shouldn't be unconditional" is important.

[Quuxplusone]

This has the problem that you're using std::is_trivially_relocatable_v but haven't checked __cpp_lib_trivially_relocatable yet. I.e., this would fail if compiled on an implementation where the compiler supports [[trivially_relocatable]] and __is_trivially_relocatable but the library doesn't implement is_trivially_relocatable_v and uninitialized_relocate.
So I suggest that this should instead be guarded by

#if defined(__cpp_lib_trivially_relocatable)

and just eliminate the XYZ_INDIRECT_IS_TRIVIALLY_RELOCATABLE macro entirely.

[Quuxplusone]

Also, if you have any tests for non-std::allocator allocators, it would be good to check here that one of those is not trivially relocatable. For example https://p1144.godbolt.org/z/TTG1r6d71

using A = boost::interprocess::allocator<int,
  boost::interprocess::managed_shared_memory::segment_manager>;
static_assert(!std::is_trivially_relocatable_v<xyz::indirect<int, A>>);

[Quuxplusone]

This shouldn't be unconditional; it depends on the allocator. So the building block above should be

#define XYZ_TRIVIALLY_RELOCATABLE_IF(x) [[trivially_relocatable(x)]]

and then here should be either something simple-and-conservative:

class XYZ_TRIVIALLY_RELOCATABLE_IF((std::is_same_v<A, std::allocator<T>>)) indirect {

or else something like libc++'s implementation of vector:

template <class A>
struct allocator_pocma_models_relocatable : std::bool_constant<
    allocator_traits<A>::is_always_equal::value ||
    (allocator_traits<A>::propagate_on_container_copy_assignment::value &&
     allocator_traits<A>::propagate_on_container_move_assignment::value &&
     allocator_traits<A>::propagate_on_container_swap::value)
> {};

template <class A>
inline constexpr bool indirect_be_trivially_relocatable_v =
    std::is_trivially_relocatable_v<A> &&
    std::is_trivially_relocatable_v<typename allocator_traits<A>::pointer> &&
    allocator_pocma_models_relocatable<A>::value;

template<class T, class A = std::allocator<T>>
class XYZ_TRIVIALLY_RELOCATABLE_IF(indirect_be_trivially_relocatable_v<A>) indirect {

[Quuxplusone]

Btw, to explain my naming choice here: Because of the precedent set by <type_traits>, I think of foo_is_bar as asking a question — therefore I needed a different convention for making an order. Hence foo_be_bar (or sometimes foo_should_be_bar, but that's a longer identifier): read "Foo, be bar!" So foo_be_bar determines the Platonic barness of foo, which in turn determines the value of foo_is_bar.
Not that I'm insisting you should adopt this convention; just explaining in case it looked like a typo. ;)

[jbcoe]

@Quuxplusone Thanks for the great review.

One query: if we make trivially relocatable conditional as you've suggested, I think we lose the support for incomplete types no?

[Quuxplusone]

One query: if we make trivially relocatable conditional as you've suggested, I think we lose the support for incomplete types no?

I didn't think of that, but I didn't intend to, and I don't think it happens. xyz::indirect<Incomplete> should still work fine. It's true that xyz::indirect<int, IncompleteAllocator<int>> will not work, but that already didn't work anyway because you need A to be complete in order to find out what typename A::pointer is.

I encourage you to add test coverage for xyz::indirect<Incomplete> if that use-case is important to you!

[jbcoe]

I'll close this as we don't want to merge until relocation in C++ is a thing.

[Quuxplusone]

I'll close this as we don't want to merge until relocation in C++ is a thing.

FWIW, as an interested observer, I would prefer to see this taken to completion and merged. It's all properly guarded by the appropriate #ifs (or at least it should be, and I'm happy to take another look), so it's not harmful to have the codepaths in your main branch (except that it is added complexity, I'd admit). Meanwhile

• it makes your library work "out of the box" with relocatability-enabled compilers, e.g. my Clang fork; you can test this on Godbolt or natively
• it serves as an example (for WG21 and for other library maintainers) of how the proposal would work in practice
• it serves as an example (for WG21) that your indirect/polymorphic proposal doesn't conflict with relocatability

"Merge to trunk, under the proper #ifs" is also the route taken by e.g. charles-salvia/std_error (using the feature-test macros) and STEllAR-GROUP/hpx (using its own HPX_HAS_P1144_RELOCATE_AT macro).

(But it shouldn't be merged immediately as-is, because as you can see from Godbolt there's at least two typos in the #if'ed-out parts; that's certainly part of the argument in the "complexity" column.)

[jbcoe]

Thanks @Quuxplusone . For now the complexity hit is more than I want. I'm trying to minimise cognitive load as we get the paper and reference implementation ready. Feel free to fork.