prevector: enforce is_trivially_copyable_v #24962

        std::memmove(first.ptr, last.ptr, endp - reinterpret_cast<char*>(last.ptr));

Maybe switch to std::memmove, which comes with the documentation:

https://en.cppreference.com/w/cpp/string/byte/memmove

If the objects are potentially-overlapping or not TriviallyCopyable, the behavior of memmove is not specified and may be undefined.

I'll change all occurrences of memmove in that file, plus memcpy as well, right? Then I'll include cstring instead of string.h

Oh, if the changes are more expensive, then maybe do it in another commit or pull request.

Does the UB section of the documentation in https://en.cppreference.com/w/cpp/string/byte/memmove (C++ memmove) apply to https://en.cppreference.com/w/c/string/byte/memmove (C memmove)?

In the C version:

The behavior is undefined if access occurs beyond the end of the dest array.

The behavior is undefined if either dest or src is an invalid or null pointer. <-- also specified in the C++ memmove doc

The behavior is undefined if the size of the character array pointed to by dest < count <= destsz; in other words, an erroneous value of destsz does not expose the impending buffer overflow.

In the C++ version:

If the objects are potentially-overlapping or not TriviallyCopyable, the behavior of memmove is not specified and may be undefined. <-- not stated in the C version

I am not familiar with C, but maybe every class is trivially copyable in C?

C doesn't have classes.

Right (interesting historic context here). So IIUC the premise in the pull description only applies to the C++ memmove and it may make sense to change to it at the same time (or not do this change at all).

the premise in the pull description only applies to the C++ memmove and it may make sense to change to it at the same time (or not do this change at all).

I still think it is necessary to do the change, even when we stay with memmove. The current implementation just does not work with complex objects. E.g. here is a small example:

BOOST_AUTO_TEST_CASE(Erase) {
    prevector<8, std::string> pv;
    pv.push_back("a");
    pv.push_back("b");
    pv.erase(pv.begin());
    pv.push_back("X");
    std::cout << pv.front() << std::endl;
}

One would expect that this prints "b", as "a" was deleted and "b" should now be the first element. But l get this output:

Running 1 test case...
X
free(): invalid pointer
unknown location(0): fatal error: in "prevector_tests/Erase": signal: SIGABRT (application abort requested)

So suddenly the first element is not "b" but "X". This is due to to small string optimization, the strings point to member data, and this is simply copied. Complex objects have to be moved for this to work.

here is another motivating example with std::swap:

BOOST_AUTO_TEST_CASE(NonTrivial) {
    prevector<8, std::string> a;
    prevector<8, std::string> b;
    a.push_back("a");
    b.push_back("b");

    std::cout << "a=" << a.front() << ", b=" << b.front() << std::endl;
    std::swap(a, b);
    std::cout << "a=" << a.front() << ", b=" << b.front() << std::endl;
}

Running that test produces this output:

Running 1 test case...
a=a, b=b
a=a, b=b
free(): invalid pointer
unknown location(0): fatal error: in "prevector_tests/NonTrivial": signal: SIGABRT (application abort requested)

So swap doesn't seem to do anything for these objects, and destructing will cause a SIGABRT.

I think we should either restrict prevector to only work with supported types, or to correctly implement it for nontrivial types. Fixing this is quite a bit more work though, and would need a lot more testing.

Somewhat followed up in #25172.

Out of interest, I am wondering to what extend trivially copyable is related to trivially destructible?

Per the standard a class is only trivially copyable when it also has a trivial destructor: https://eel.is/c++draft/class#prop-1.3