Page MenuHomePhabricator

P1144 "Trivially relocatable" (1/3): is_trivially_relocatable, relocate_at, and uninitialized_relocate
Needs RevisionPublic

Authored by Quuxplusone on May 9 2019, 2:32 PM.

Details

Summary

Implement "trivially relocatable" traits and algorithms.

This implements the library behavior proposed in P1144R3. This patch doesn't do any special optimizations (e.g. to std::vector::reserve) nor add warrants to libc++ class types; those will come later. This is just the bare minimum to conform to P1144's additions to the standard.

We don't mangle relocate_at into __relocate_at, etc., although we could if we want to hide it from end-users.

We don't mangle is_trivially_relocatable into __is_trivially_relocatable, nor can we, because that's the name of the compiler builtin in my Clang patch. Same reason we can't mangle is_trivially_constructible into __is_trivially_constructible. If we want to hide is_trivially_relocatable from end-users, we should come up with a new convention, such as __ext_is_trivially_relocatable.

D50119 adds __has_extension(__is_trivially_relocatable) to the compiler.

TODO:

  • hide names? how?
  • any reason to gate the new names on any particular _LIBCPP_STD_VER, such as 17? (I think not.)
  • split up the tests for uninitialized_relocate{,_n} into three or four files each?
  • write a test for relocate_at

Diff Detail

Repository
rCXX libc++

Event Timeline

Quuxplusone created this revision.May 9 2019, 2:32 PM
Quuxplusone edited the summary of this revision. (Show Details)May 9 2019, 2:36 PM
jfb added a subscriber: rjmccall.
zoecarver added inline comments.
include/memory
3463

Nit: is _IsMemcpyable guaranteed to be true_type or false_type? If so, maybe make it specifically false_type.

3510

I think this should require _LIBCPP_STD_VER > 03.

3556

What is the reason for is_volatile? I don't see that in the paper (but maybe I am missing something).

test/libcxx/type_traits/is_trivially_relocatable.pass.cpp
34

Maybe add a test for things that are experimental?

test/std/utilities/meta/meta.unary/meta.unary.prop/is_trivially_relocatable.pass.cpp
15

Shouldn't this also contain c++98?

Quuxplusone marked 5 inline comments as done.May 11 2019, 4:54 PM
Quuxplusone added inline comments.
include/memory
3463

Brain fart on my part. (This isn't a class template with a partial specialization, it's two overloaded function templates!) Will fix.

3466

I'm not sure whether this launder is doing anything.
Alternatively, should I assign __dest = ::new (... and then return __dest?

3510

libc++ supports std::move even in _LIBCPP_HAS_NO_RVALUE_REFERENCES-mode, presumably because it would be too tedious to #ifdef all the places where we use std::move (such as here).

3556

It's related to https://quuxplusone.github.io/blog/2018/07/13/trivially-copyable-corner-cases/ — if we have a range of volatile Foo, then the user is probably expecting us to follow the abstract machine and copy the Foo objects one by one, instead of byte-by-byte (possible tearing). The library doesn't technically have to follow the abstract machine here, but I think it's QoI to do so.

I just re-examined std::copy and it doesn't bother to check is_volatile, so I guess it would be consistent for libc++ not to check is_volatile here either. (That would make the QoI consistently low, IMHO.)

test/std/utilities/meta/meta.unary/meta.unary.prop/is_trivially_relocatable.pass.cpp
15

Hmm. My impression had been that c++98 and c++03 were synonymous, and that the tests only ever passed c++03. But now I see that most tests do refer to c++98, c++03, except for the ones I added and also

test/std/diagnostics/syserr/is_error_code_enum.pass.cpp
test/std/input.output/iostreams.base/is_error_code_enum_io_errc.pass.cpp
test/std/utilities/time/time.duration/time.duration.alg/abs.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/ceil.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/floor.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/round.fail.cpp
test/std/utilities/time/time.point/time.point.cast/ceil.fail.cpp
test/std/utilities/time/time.point/time.point.cast/floor.fail.cpp
test/std/utilities/time/time.point/time.point.cast/round.fail.cpp

Should these other tests also get c++98 added, or should c++98 and c++03 be synonymous in tests?

Fixed brain fart. Added UNSUPPORTED: c++98 to test.

zoecarver added inline comments.May 12 2019, 11:01 AM
include/memory
3464

Are you intentionally not doing anything with the result of new? Also, I am not sure you need to static_cast to void*.

Also, also, I think you want to move __source not __dest (because I am pretty sure __dest is where you want to relocate the new thing and __source is where you want to get it).

3466

That seems more correct. After calling new the pointer should still be active, so you won't need launder .

I think launder is only necessary after memmove because then the pointer's lifetime is over.

3481

I think this should be is_trivially_relocatable<_Tp>().

3510

Good to know.

3556

Maybe open an issue about this (in bugzilla) so that once/if P1153 is introduced it can be updated.

zoecarver added inline comments.May 12 2019, 11:25 AM
include/memory
3518

I don't think this overload is necessary in addition to the below one.

Quuxplusone marked 10 inline comments as done.

Add a heterogeneous __relocate_at2 helper for uninitialized_relocate.
Also fix a major typo-bug in relocate_at; thanks @zoecarver!

include/memory
3464

Yikes! That was a pretty bad bug. Thanks.

3518

I think this overload would never really be useful in practice, but since it does something different from the below one (element-wise memcpy in a loop, instead of one big memcpy), I figured I should implement it.

However, since P1144R3 adds relocate_at, I can refactor this overload and the one above into a single overload that does relocate_at-in-a-loop! Will do.

In the process of rewriting this code, I realized that whereas uninitialized_relocate copies uninitialized_move in not requiring the two iterator types to have the same value_type, relocate_at requires the exact same type _Tp * for both __source and __dest. I have added an internal version __relocate_at2(_Sp *__source, _Dp *__dest), and am ambivalently considering adding it to the proposal P1144R4. That would require some kind of change to the word "homogeneous" in P1144R3, though. Feel free to comment on this idea here and/or by email.

zoecarver added inline comments.May 12 2019, 8:10 PM
include/memory
3464

Of course! That's what code review is for :)

Maybe add a test for this though.

3518

IMVHO I think it would be better to have them be one type and let the user static_cast the pointer if they want something different.

If the difference is meaningful, maybe another overload should be added in P1144R3. Otherwise, I think combining them and using relocate_at is a good idea.

Quuxplusone marked an inline comment as done.May 12 2019, 10:16 PM
Quuxplusone added inline comments.
include/memory
3518

I think I don't understand what you thought I meant. :)
By "homogeneous" relocate_at, I mean it can only relocate from T to that same type T (not even with different cv-qualifiers). As opposed to uninitialized_relocate, which is heterogeneous just like uninitialized_move and std::copy. For example:

long *src;
double *dest;
std::uninitialized_relocate(src, src+n, dest);  // OK
std::relocate_at(src, dest);  // not OK, because the types differ
std::__relocate_at2(src, dest);  // OK

The above case uses a non-trivial relocation operation.

int *src;
std::unique_ptr<int> *dest;
std::uninitialized_relocate(src, src+n, dest);  // OK
std::relocate_at(src, dest);  // not OK, because the types differ
std::__relocate_at2(src, dest);  // OK

The above case is functionally equivalent to memcpy, although this patch won't go so far as to actually make it use memcpy.

zoecarver added inline comments.May 13 2019, 11:28 AM
include/memory
3518

I understand what you meant. I think it is important that (for the most part) pointer functions are "homogeneous", especially the ones that rely on sizeof.

Here is an example of what I mean:

template<class T>
void cpy(T* dst, T* src)
{
	std::memcpy(dst, src, sizeof(T));
}

int main()
{
	int x = 42;
	int* iptr = &x;
	void* vptr = &x;
	cpy(iptr, vptr); // fails
	cpy(iptr, static_cast<int*>(vptr)); // works
}

The above function will prevent easy run time out of bounds errors from differently sized types.

Context for other reviewers:

I spoke to Arthur at C++Now after his talk, and my opinion was that it would be useful to implement the trivially relocatable proposal as a conforming extension to libc++ to allow gathering some usage experience. However, I am dead against making any change that is user visible. So what I would like is for us to implement the traits and algorithms but use reserved identifiers -- this way we can implement optimizations inside the library without changing the user-visible API. And if the proposal never makes it into C++, we could even remove everything that has to do with trivially relocatable since it was only an internal optimization anyway -- that's the spirit.

Y'all know I'm against extensions, however I think gathering usage experience is useful and this is an opportunity to do so without making user-visible changes, which is one case where I think extensions are easy to sell. Please LMK if you disagree, otherwise I told Arthur I would help shepherd the patch through (the libc++ part).

  • hide names? how?

Use __libcpp_xxx or whatever scheme you like. Those are implementation details anyway, we can change them as we see fit.

  • any reason to gate the new names on any particular _LIBCPP_STD_VER, such as 17? (I think not.)

Good question. On the one hand, you want to get as much usage experience as possible, so that would mean enabling it on the oldest possible standard. On the other hand, once (if) the proposal makes it into C++, we won't want to implement the functionality in older standards. But removing the optimizations from C++03 will cause regressions, which is bad.

  • split up the tests for uninitialized_relocate{,_n} into three or four files each?

The tests look reasonable to me as-is. However, they should all be in test/libcxx/ because they are libc++ specific.

  • write a test for relocate_at

Please do.

ldionne requested changes to this revision.May 14 2019, 1:59 PM

Requesting change so it shows up properly in my review queue.

This revision now requires changes to proceed.May 14 2019, 1:59 PM