Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Fix no-op shrink_to_fit for vector<bool> #120495

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −8
Open

[libc++] Fix no-op shrink_to_fit for vector<bool> #120495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions libcxx/include/__vector/vector_bool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,11 +859,13 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::reserve(size_type _

template <class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::shrink_to_fit() _NOEXCEPT {
if (__external_cap_to_internal(size()) > __cap_) {
if (__external_cap_to_internal(size()) < __cap_) {
#if _LIBCPP_HAS_EXCEPTIONS
try {
#endif // _LIBCPP_HAS_EXCEPTIONS
vector(*this, allocator_type(__alloc_)).swap(*this);
vector __v(*this, allocator_type(__alloc_));
if (__v.__cap_ < __cap_)
__v.swap(*this);
#if _LIBCPP_HAS_EXCEPTIONS
} catch (...) {
}
Expand Down
45 changes: 39 additions & 6 deletions libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@

// void shrink_to_fit();

// XFAIL: FROZEN-CXX03-HEADERS-FIXME

#include <cassert>
#include <climits>
#include <vector>

#include "increasing_allocator.h"
Expand All @@ -20,19 +23,49 @@

TEST_CONSTEXPR_CXX20 bool tests() {
{
std::vector<bool> v(100);
using C = std::vector<bool>;
C v(100);
v.push_back(1);
v.clear();
v.shrink_to_fit();
assert(v.capacity() >= 101);
assert(v.size() >= 101);
assert(v.capacity() == 0);
assert(v.size() == 0);
}
#if TEST_STD_VER >= 11
{
std::vector<bool, min_allocator<bool>> v(100);
using C = std::vector<bool, min_allocator<bool> >;
C v(100);
v.push_back(1);
C::size_type before_cap = v.capacity();
v.shrink_to_fit();
assert(v.capacity() >= 101);
assert(v.size() >= 101);
assert(v.capacity() <= before_cap);
assert(v.size() == 101);
}

#if defined(_LIBCPP_VERSION)
{
using C = std::vector<bool>;
unsigned bits_per_word = static_cast<unsigned>(sizeof(C::__storage_type) * CHAR_BIT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part of the test suite located under libcxx/test/std is intended to be portable and usable by other implementations (like MSVC STL or libstdc++). As such, we don't use things specific to libc++ like C::__storage_type. Or if we absolutely must, we either:

  1. Put that part of the test under libcxx/test/libcxx, which contains tests that are not entirely portable, or
  2. Guard the check with #if defined(_LIBCPP_VERSION), or
  3. Use something like LIBCPP_ASSERT from test_macros.h.

Which of those three we do depends on the amount of libc++ specific test code we're trying to add, maintenance burden, etc.

In this case, the very best would be to figure out the number of bits per word via some public-facing API if that's doable, but I'm not certain it is. Otherwise, I would probably go for #if defined(_LIBCPP_VERSION) around those two tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your insightful feedback. The three options you mentioned have provided me with some new and valuable information that I wasn't previously aware of.

Among the options, I believe using #if defined(_LIBCPP_VERSION) around the two tests that use C::__storage_type would be the most appropriate for my purpose. This is because libc++ and libstdc++ have different word sizes (libc++ uses Alloc::size_type, which is typically 64 bits, while libstdc++ uses unsigned long). Moreover, I don't find a way to extract this information from a public API. Even if I could, the decision on whether to shrink would be implementation-dependent as the request to shrink is non-binding. Therefore, I choose to use the #if defined(_LIBCPP_VERSION) option you suggested.

C v(bits_per_word);
v.push_back(1);
assert(v.capacity() == bits_per_word * 2);
assert(v.size() == bits_per_word + 1);
v.pop_back();
v.shrink_to_fit();
assert(v.capacity() == bits_per_word);
assert(v.size() == bits_per_word);
}
{
using C = std::vector<bool>;
unsigned bits_per_word = static_cast<unsigned>(sizeof(C::__storage_type) * CHAR_BIT);
C v;
v.reserve(bits_per_word * 2);
v.push_back(1);
assert(v.capacity() == bits_per_word * 2);
assert(v.size() == 1);
v.shrink_to_fit();
assert(v.capacity() == bits_per_word);
assert(v.size() == 1);
}
#endif

Expand Down
Loading