[libc++] Fix ambiguous call to std::max in vector<bool>

[winner245]

Closes #121713.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This PR addresses a compilation error in vector<bool> that arises from integral promotion when using custom allocators with smaller integral types, such as std::uint16_t. It also optimizes similar code in std::basic_string to prevent potential issues related to integral promotions.

The following code snippet reproduces the bug in vector<bool>:

#include <exception>
#include <iostream>
#include <limits>
#include <memory>
#include <vector>

template <typename T, typename SIZE_TYPE = std::size_t, typename DIFF_TYPE = std::ptrdiff_t>
class CustomSizedAllocator {
  template <typename U, typename Sz, typename Diff>
  friend class CustomSizedAllocator;

public:
  using value_type                  = T;
  using size_type                   = SIZE_TYPE;
  using difference_type             = DIFF_TYPE;
  using propagate_on_container_swap = std::true_type;

  explicit CustomSizedAllocator(int i = 0) : data_(i) {}

  template <typename U, typename Sz, typename Diff>
  constexpr CustomSizedAllocator(const CustomSizedAllocator<U, Sz, Diff>& a) noexcept : data_(a.data_) {}

  constexpr T* allocate(size_type n) {
    if (n > max_size())
      throw std::bad_array_new_length();
    return std::allocator<T>().allocate(n);
  }

  constexpr void deallocate(T* p, size_type n) noexcept { std::allocator<T>().deallocate(p, n); }

  constexpr size_type max_size() const noexcept { return std::numeric_limits<size_type>::max() / sizeof(value_type); }

  int get() { return data_; }

private:
  int data_;

  constexpr friend bool operator==(const CustomSizedAllocator& a, const CustomSizedAllocator& b) {
    return a.data_ == b.data_;
  }
  constexpr friend bool operator!=(const CustomSizedAllocator& a, const CustomSizedAllocator& b) {
    return a.data_ != b.data_;
  }
};


int main() {
  using Alloc = CustomSizedAllocator<bool, std::uint16_t, std::int16_t>;
  std::vector<bool, Alloc> c{Alloc{1}};
  c.resize(10);

  return 0;
}

This code fails to compile in libc++, while it compiles successfully in other standard library implementations. The underlying issue is found in the following line of code within vector<bool>:

return std::max(2 * __cap, __align_it(__new_size));

where the first operand's result type is promoted to int, while the second operand retains the size_type type, which is std::uint16_t in this case. This mismatch in argument types for std::max<_Tp> leads to the failure of template type argument deduction.

The solution is straightforward: explicitly specify the template type for std::max to ensure consistent types for both operands.

---

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

4 Files Affected:

• (modified) libcxx/include/__cxx03/string (+6-2)
• (modified) libcxx/include/__cxx03/vector (+1-1)
• (modified) libcxx/include/__vector/vector_bool.h (+1-1)
• (modified) libcxx/include/string (+3-1)

diff --git a/libcxx/include/__cxx03/string b/libcxx/include/__cxx03/string
index c4431dcb04d41e..c29f74290bd416 100644
--- a/libcxx/include/__cxx03/string
+++ b/libcxx/include/__cxx03/string
@@ -2483,7 +2483,9 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
     __throw_length_error();
   pointer __old_p = __get_pointer();
   size_type __cap =
-      __old_cap < __ms / 2 - __alignment ? __recommend(std::max(__old_cap + __delta_cap, 2 * __old_cap)) : __ms - 1;
+      __old_cap < __ms / 2 - __alignment
+          ? __recommend(std::max<size_type>(__old_cap + __delta_cap, 2 * __old_cap))
+          : __ms - 1;
   __annotate_delete();
   auto __allocation = std::__allocate_at_least(__alloc(), __cap + 1);
   pointer __p       = __allocation.ptr;
@@ -2526,7 +2528,9 @@ _LIBCPP_DEPRECATED_("use __grow_by_without_replace") basic_string<_CharT, _Trait
     __throw_length_error();
   pointer __old_p = __get_pointer();
   size_type __cap =
-      __old_cap < __ms / 2 - __alignment ? __recommend(std::max(__old_cap + __delta_cap, 2 * __old_cap)) : __ms - 1;
+      __old_cap < __ms / 2 - __alignment
+          ? __recommend(std::max<size_type>(__old_cap + __delta_cap, 2 * __old_cap))
+          : __ms - 1;
   __annotate_delete();
   auto __allocation = std::__allocate_at_least(__alloc(), __cap + 1);
   pointer __p       = __allocation.ptr;
diff --git a/libcxx/include/__cxx03/vector b/libcxx/include/__cxx03/vector
index 6ee35b4e36258f..80da74ded67b7a 100644
--- a/libcxx/include/__cxx03/vector
+++ b/libcxx/include/__cxx03/vector
@@ -2328,7 +2328,7 @@ vector<bool, _Allocator>::__recommend(size_type __new_size) const {
   const size_type __cap = capacity();
   if (__cap >= __ms / 2)
     return __ms;
-  return std::max(2 * __cap, __align_it(__new_size));
+  return std::max<size_type>(2 * __cap, __align_it(__new_size));
 }
 
 //  Default constructs __n objects starting at __end_
diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 36eb7f350ac406..26b172af9138db 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -527,7 +527,7 @@ vector<bool, _Allocator>::__recommend(size_type __new_size) const {
   const size_type __cap = capacity();
   if (__cap >= __ms / 2)
     return __ms;
-  return std::max(2 * __cap, __align_it(__new_size));
+  return std::max<size_type>(2 * __cap, __align_it(__new_size));
 }
 
 //  Default constructs __n objects starting at __end_
diff --git a/libcxx/include/string b/libcxx/include/string
index 17bf4b3b98bf34..911b67cfcf26e8 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -2518,7 +2518,9 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
     __throw_length_error();
   pointer __old_p = __get_pointer();
   size_type __cap =
-      __old_cap < __ms / 2 - __alignment ? __recommend(std::max(__old_cap + __delta_cap, 2 * __old_cap)) : __ms - 1;
+      __old_cap < __ms / 2 - __alignment
+          ? __recommend(std::max<size_type>(__old_cap + __delta_cap, 2 * __old_cap))
+          : __ms - 1;
   __annotate_delete();
   auto __guard      = std::__make_scope_guard(__annotate_new_size(*this));
   auto __allocation = std::__allocate_at_least(__alloc_, __cap + 1);

[frederick-vs-ja]

I think we can add some simple test cases.

[winner245]

It appears that my added tests for size_allocator<T, SIZE_TYPE, DIFF_TYPE> have triggered a new bug in all the __bit_iterator algorithms, including std::{copy, copy_n, copy_backward, fill, fill_n, find, count} and their ranges algorithm equivalents. The bug is essentially caused by an integral promotion of unsigned integral types (std::uint8_t, std::uint16_t) to signed int, followed by a left shift of the signed negative values, which is UB before C++20. Additionally, there are also some undesired integral promotions of small unsigned integral types to int, followed by right-shifts, which are implementation-defined before C++20. Therefore, we need to fix this new bug in these algorithms as well.

[ldionne]

Nice catch! I have some comments.

Requesting changes since IIUC other fixes need to go in before we can merge this.

[ldionne]

LGTM with minor comments!