inlined_vector: Use trivial relocation for erase

[Quuxplusone]

Prior art for the vector::erase optimization:
https://github.com/AmadeusITGroup/amc/blob/efcb7be/include/amc/vectorcommon.hpp#L176-L180
https://github.com/bloomberg/bde/blob/e15f05be6/groups/bsl/bslalg/bslalg_arrayprimitives.h#L3787-L3799
https://github.com/facebook/folly/blob/d24bf04/folly/FBVector.h#L1254-L1262
https://github.com/qt/qtbase/blob/fbfee2d/src/corelib/tools/qarraydataops.h#L856-L861

attn @derekmauro @jacobsa

[Quuxplusone]

@derekmauro : Hm, I figured out (by building locally) that the earlier build failure was that I'd typoed absl::is_trivially_copyable instead of std::is_trivially_copyable; but (now that Abseil builds clean for me locally) I'm going to need a hint to figure out the reason those CI builds are failing. (The hyperlinks go to fusion2-something, which I assume is Google internal.)

[derekmauro]

I know there is a way to make the build links public but I think there is some work involved. I'll just copy the error for you.

absl/meta/type_traits_test.cc:791:17: error: static assertion failed due to requirement '!absl::is_trivially_relocatable<S>::value': 
  791 |   static_assert(!absl::is_trivially_relocatable<S>::value, "");
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
absl/meta/type_traits_test.cc:802:17: error: static assertion failed due to requirement '!absl::is_trivially_relocatable<S>::value': 
  802 |   static_assert(!absl::is_trivially_relocatable<S>::value, "");
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Quuxplusone]

Oh, I see! My brain mismatched the parens in

#if ABSL_HAVE_BUILTIN(__is_trivially_relocatable) &&                 \
    !(defined(__clang__) && (defined(_WIN32) || defined(_WIN64))) && \
    !(defined(__APPLE__)) && !defined(__NVCC__)

Abseil actually is using Clang's __is_trivially_relocatable on non-Windows non-Mac platforms. My patch was predicated on Abseil's not using Clang's __is_trivially_relocatable at all (precisely because it mishandles those two static-asserts — so I guess my new test coverage was a good idea! ;)). The simplest way forward would be to simply replace that whole #if with

#if 0 // sad face, hope to re-enable for Clang 19+

or at best

#if ABSL_HAVE_BUILTIN(__is_trivially_relocatable) && defined(__cpp_impl_trivially_relocatable)
  // the specific spelling of the builtin type-trait *plus* the feature-test macro proposed in P1144 

Would either of those be acceptable here?

[derekmauro]

I think this PR is addressing 2 issues:

1. Bugs in the implementation of absl::is_trivially_relocatable.
2. Using trivial relocation for absl::InlinedVector::erase.

Would it be possible to split this into two PRs to address these two issues separately?

Both changes look fine on their own, though I will have to run them though the global tests before importing.

[Quuxplusone]

I think this PR is addressing 2 issues:

1. Bugs in the implementation of absl::is_trivially_relocatable.
2. Using trivial relocation for absl::InlinedVector::erase.

Would it be possible to split this into two PRs to address these two issues separately?

Definitely, will do. (I'll create a new PR for "bugs in the implementation/tests" and leave this one's title accurate.)
It's worth mentioning that the "bugs in the implementation" really start mattering only after the vector::erase and/or vector::swap optimizations are applied. As long as Abseil wasn't using the type-trait for optimization, the "bugs" in it were more of an academic question. :) Anyway, I'm thrilled this is moving forward!

[Quuxplusone]

@derekmauro: Okay, I've split out #1625 as a first step / prerequisite. Now I'm going to update this one (#1615) to just optimize erase — but be aware it'll basically depend for correctness on having #1625 first.

[derekmauro]

This is now triggering a GCC warning:

In file included from absl/container/inlined_vector_test.cc:15:
In member function 'absl::ns::InlinedVector<T, N, A>::iterator absl::ns::InlinedVector<T, N, A>::erase(const_iterator) [with T = {anonymous}::MoveOnly; long unsigned int N = 2; A = std::allocator<{anonymous}::MoveOnly>]',
    inlined from 'virtual void {anonymous}::InlinedVectorTest_MoveOnly_Test::TestBody()' at absl/container/inlined_vector_test.cc:435:10:
./absl/container/inlined_vector.h:778:26: error: 'v' may be used uninitialized [-Werror=maybe-uninitialized]
  778 |     return storage_.Erase(pos, pos + 1);
      |            ~~~~~~~~~~~~~~^~~~~~~~~~~~~~
In file included from ./absl/container/inlined_vector.h:53:
./absl/container/internal/inlined_vector.h: In member function 'virtual void {anonymous}::InlinedVectorTest_MoveOnly_Test::TestBody()':
./absl/container/internal/inlined_vector.h:888:6: note: by argument 2 of type 'absl::ns::inlined_vector_internal::ConstIterator<std::allocator<{anonymous}::MoveOnly> >' {aka 'const {anonymous}::MoveOnly*'} to 'absl::ns::inlined_vector_internal::Iterator<A> absl::ns::inlined_vector_internal::Storage<T, N, A>::Erase(absl::ns::inlined_vector_internal::ConstIterator<A>, absl::ns::inlined_vector_internal::ConstIterator<A>) [with T = {anonymous}::MoveOnly; long unsigned int N = 2; A = std::allocator<{anonymous}::MoveOnly>]' declared here
  888 | auto Storage<T, N, A>::Erase(ConstIterator<A> from, ConstIterator<A> to)
      |      ^~~~~~~~~~~~~~~~
absl/container/inlined_vector_test.cc:431:36: note: 'v' declared here
  431 |   absl::InlinedVector<MoveOnly, 2> v;
      |                                    ^

I've seen this before in this file and I concluded it was a false positive, but I haven't had a chance to look at it closely yet. This is with GCC 13.2 and seems to require an optimized build to trigger. If you have a Linux machine with Docker, you can try it yourself with ci/linux_gcc-latest_libstdcxx_bazel.sh, otherwise I'll try to take a look soon.

[derekmauro]

Sigh, I've completely failed at github. I tried resolving a merge conflict, screwed that up, and then somehow reverted everything, which caused github to close this. Mind opening a new PR? Sorry about this.