[libc++] Fix __split_buffer::reserve()

[PaulXiCao]

The function __split_buffer::reserve() did not reserve when instructed to.

Open questions about the tests:

• Do we implement unit tests for internal data structures at all?
• Is there another file I should include the tests into?
• Is there a more suitable folder to include the newly created test file into?

Note: This issue came up while working on #105379.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: None (PaulXiCao)

Changes

The function __split_buffer::reserve() did not reserve when instructed to.

Open questions about the tests:

• Do we implement unit tests for internal data structures at all?
• Is there another file I should include the tests into?
• Is there a more suitable folder to include the newly created test file into?

Note: This issue came up while working on #105379.

---

Full diff: https://github.com/llvm/llvm-project/pull/105681.diff

2 Files Affected:

• (modified) libcxx/include/__split_buffer (+1-1)
• (added) libcxx/test/libcxx/utilities/split_buffer.pass.cpp (+69)

diff --git a/libcxx/include/__split_buffer b/libcxx/include/__split_buffer
index bab724d1b8963b..381e3d2bc7b7c8 100644
--- a/libcxx/include/__split_buffer
+++ b/libcxx/include/__split_buffer
@@ -430,7 +430,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::swap(__split
 
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::reserve(size_type __n) {
-  if (__n < capacity()) {
+  if (__n > capacity()) {
     __split_buffer<value_type, __alloc_rr&> __t(__n, 0, __alloc());
     __t.__construct_at_end(move_iterator<pointer>(__begin_), move_iterator<pointer>(__end_));
     std::swap(__first_, __t.__first_);
diff --git a/libcxx/test/libcxx/utilities/split_buffer.pass.cpp b/libcxx/test/libcxx/utilities/split_buffer.pass.cpp
new file mode 100644
index 00000000000000..6ce361508fd2d2
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/split_buffer.pass.cpp
@@ -0,0 +1,69 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <__split_buffer>
+#include <__config>
+#include <cassert>
+#include <memory>
+#include <type_traits>
+#include <string>
+
+#include "test_macros.h"
+
+struct simple_test_type {
+  int value;
+};
+
+struct complex_test_type {
+  std::string value;
+  complex_test_type(const std::string& v) : value(v) {}
+};
+
+template <class TEST_TYPE>
+_LIBCPP_CONSTEXPR_SINCE_CXX20 int capacity_after_initialization() {
+  std::__split_buffer<TEST_TYPE> sb;
+  return sb.capacity();
+}
+
+template <class TEST_TYPE>
+_LIBCPP_CONSTEXPR_SINCE_CXX20 int capacity_after_reserve(const std::size_t n) {
+  std::__split_buffer<TEST_TYPE> sb;
+  sb.reserve(n);
+  return sb.capacity();
+}
+
+int main() {
+  { // check test_types' features
+#if _LIBCPP_STD_VER >= 17
+    static_assert(std::is_trivially_copyable_v<simple_test_type>);
+    static_assert(not std::is_trivially_copyable_v<complex_test_type>);
+#endif
+  }
+
+  { // test simple_test_type at run-time
+    assert(0 == capacity_after_initialization<simple_test_type>());
+    assert(42 == capacity_after_reserve<simple_test_type>(42));
+  }
+
+  { // test complex_test_type at run-time
+    assert(0 == capacity_after_initialization<complex_test_type>());
+    assert(42 == capacity_after_reserve<complex_test_type>(42));
+  }
+
+#if _LIBCPP_STD_VER >= 20
+  { // test simple_test_type at compile-time
+    static_assert(0 == capacity_after_initialization<simple_test_type>());
+    static_assert(42 == capacity_after_reserve<simple_test_type>(42));
+  }
+
+  { // test complex_test_type at compile-time
+    static_assert(0 == capacity_after_initialization<complex_test_type>());
+    static_assert(42 == capacity_after_reserve<complex_test_type>(42));
+  }
+#endif
+}

[ldionne]

Is there a way to trigger this bug from a public API? Something like std::vector::push_back or something along those lines? If it's possible to trigger from a public API, I think I would favor that approach.

Do we implement unit tests for internal data structures at all?

We don't typically do that. It doesn't mean that we can't do it, but we generally stick to testing the public APIs.

Is there another file I should include the tests into?

I don't quite understand the question.

Is there a more suitable folder to include the newly created test file into?

I think the path you used is reasonable.

[philnik777]

Do we even use __split_buffer::reserve() anywhere? If not, we can just remove the function and the bug is "fixed".

[philnik777]

This has been superseded by #115735.