[libc++] Fix ambiguous calls to std::min in basic_string #132402
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Peng Liu (winner245) ChangesThe current implementation of std::min(__n, __sz - __pos);where This issue is widespread across The fix is straightforward: explicitly specify the template type argument for std::min<size_type>(__n, __sz - __pos);Closes #125187. [1] basic_string(const basic_string& str, size_type pos, size_type n,
const Allocator& a = Allocator());
basic_string& assign(const basic_string& str, size_type pos, size_type n=npos);
template <class T>
basic_string& assign(const T& t, size_type pos, size_type n=npos);
basic_string& append(const basic_string& str, size_type pos, size_type n=npos);
template <class T>
basic_string& append(const T& t, size_type pos, size_type n=npos);
basic_string& insert(size_type pos1, const basic_string& str,
size_type pos2, size_type n=npos);
template <class T>
basic_string& insert(size_type pos1, const T& t, size_type pos2, size_type n=npos);
basic_string& erase(size_type pos = 0, size_type n = npos);
basic_string& replace(size_type pos, size_type n1, const value_type* s, size_type n2);
basic_string& replace(size_type pos, size_type n1, size_type n2, value_type c);
basic_string& replace(size_type pos1, size_type n1, const basic_string& str,
size_type pos2, size_type n2=npos);
template <class T>
basic_string& replace(size_type pos1, size_type n1, const T& t,
size_type pos2, size_type n2= npos);
size_type copy(value_type* s, size_type n, size_type pos = 0) const;
template <class T>
int compare(const T& t) const noexcept;
int compare(size_type pos1, size_type n1, const value_type* s, size_type n2) const;Full diff: https://github.com/llvm/llvm-project/pull/132402.diff 2 Files Affected:
diff --git a/libcxx/include/string b/libcxx/include/string
index fa87dc2fddb59..0885362ca7e22 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1101,7 +1101,7 @@ public:
size_type __str_sz = __str.size();
if (__pos > __str_sz)
this->__throw_out_of_range();
- __init(__str.data() + __pos, std::min(__n, __str_sz - __pos));
+ __init(__str.data() + __pos, std::min<size_type>(__n, __str_sz - __pos));
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
@@ -2943,7 +2943,7 @@ basic_string<_CharT, _Traits, _Allocator>::assign(const basic_string& __str, siz
size_type __sz = __str.size();
if (__pos > __sz)
this->__throw_out_of_range();
- return assign(__str.data() + __pos, std::min(__n, __sz - __pos));
+ return assign(__str.data() + __pos, std::min<size_type>(__n, __sz - __pos));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -2957,7 +2957,7 @@ basic_string<_CharT, _Traits, _Allocator>::assign(const _Tp& __t, size_type __po
size_type __sz = __sv.size();
if (__pos > __sz)
this->__throw_out_of_range();
- return assign(__sv.data() + __pos, std::min(__n, __sz - __pos));
+ return assign(__sv.data() + __pos, std::min<size_type>(__n, __sz - __pos));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -3088,7 +3088,7 @@ basic_string<_CharT, _Traits, _Allocator>::append(const basic_string& __str, siz
size_type __sz = __str.size();
if (__pos > __sz)
this->__throw_out_of_range();
- return append(__str.data() + __pos, std::min(__n, __sz - __pos));
+ return append(__str.data() + __pos, std::min<size_type>(__n, __sz - __pos));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -3102,7 +3102,7 @@ basic_string<_CharT, _Traits, _Allocator>::append(const _Tp& __t, size_type __po
size_type __sz = __sv.size();
if (__pos > __sz)
this->__throw_out_of_range();
- return append(__sv.data() + __pos, std::min(__n, __sz - __pos));
+ return append(__sv.data() + __pos, std::min<size_type>(__n, __sz - __pos));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -3210,7 +3210,7 @@ basic_string<_CharT, _Traits, _Allocator>::insert(
size_type __str_sz = __str.size();
if (__pos2 > __str_sz)
this->__throw_out_of_range();
- return insert(__pos1, __str.data() + __pos2, std::min(__n, __str_sz - __pos2));
+ return insert(__pos1, __str.data() + __pos2, std::min<size_type>(__n, __str_sz - __pos2));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -3224,7 +3224,7 @@ basic_string<_CharT, _Traits, _Allocator>::insert(size_type __pos1, const _Tp& _
size_type __str_sz = __sv.size();
if (__pos2 > __str_sz)
this->__throw_out_of_range();
- return insert(__pos1, __sv.data() + __pos2, std::min(__n, __str_sz - __pos2));
+ return insert(__pos1, __sv.data() + __pos2, std::min<size_type>(__n, __str_sz - __pos2));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -3268,7 +3268,7 @@ basic_string<_CharT, _Traits, _Allocator>::replace(
size_type __sz = size();
if (__pos > __sz)
this->__throw_out_of_range();
- __n1 = std::min(__n1, __sz - __pos);
+ __n1 = std::min<size_type>(__n1, __sz - __pos);
size_type __cap = capacity();
if (__cap - __sz + __n1 >= __n2) {
value_type* __p = std::__to_address(__get_pointer());
@@ -3310,7 +3310,7 @@ basic_string<_CharT, _Traits, _Allocator>::replace(size_type __pos, size_type __
size_type __sz = size();
if (__pos > __sz)
this->__throw_out_of_range();
- __n1 = std::min(__n1, __sz - __pos);
+ __n1 = std::min<size_type>(__n1, __sz - __pos);
size_type __cap = capacity();
value_type* __p;
if (__cap - __sz + __n1 >= __n2) {
@@ -3346,7 +3346,7 @@ basic_string<_CharT, _Traits, _Allocator>::replace(
size_type __str_sz = __str.size();
if (__pos2 > __str_sz)
this->__throw_out_of_range();
- return replace(__pos1, __n1, __str.data() + __pos2, std::min(__n2, __str_sz - __pos2));
+ return replace(__pos1, __n1, __str.data() + __pos2, std::min<size_type>(__n2, __str_sz - __pos2));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -3361,7 +3361,7 @@ basic_string<_CharT, _Traits, _Allocator>::replace(
size_type __str_sz = __sv.size();
if (__pos2 > __str_sz)
this->__throw_out_of_range();
- return replace(__pos1, __n1, __sv.data() + __pos2, std::min(__n2, __str_sz - __pos2));
+ return replace(__pos1, __n1, __sv.data() + __pos2, std::min<size_type>(__n2, __str_sz - __pos2));
}
template <class _CharT, class _Traits, class _Allocator>
@@ -3381,7 +3381,7 @@ basic_string<_CharT, _Traits, _Allocator>::__erase_external_with_move(size_type
if (__n) {
size_type __sz = size();
value_type* __p = std::__to_address(__get_pointer());
- __n = std::min(__n, __sz - __pos);
+ __n = std::min<size_type>(__n, __sz - __pos);
size_type __n_move = __sz - __pos - __n;
if (__n_move != 0)
traits_type::move(__p + __pos, __p + __pos + __n, __n_move);
@@ -3555,7 +3555,7 @@ basic_string<_CharT, _Traits, _Allocator>::copy(value_type* __s, size_type __n,
size_type __sz = size();
if (__pos > __sz)
this->__throw_out_of_range();
- size_type __rlen = std::min(__n, __sz - __pos);
+ size_type __rlen = std::min<size_type>(__n, __sz - __pos);
traits_type::copy(__s, data() + __pos, __rlen);
return __rlen;
}
@@ -3609,7 +3609,7 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 int basic_string<_CharT, _Traits, _Allocato
size_type __sz = size();
if (__pos1 > __sz || __n2 == npos)
this->__throw_out_of_range();
- size_type __rlen = std::min(__n1, __sz - __pos1);
+ size_type __rlen = std::min<size_type>(__n1, __sz - __pos1);
int __r = traits_type::compare(data() + __pos1, __s, std::min(__rlen, __n2));
if (__r == 0) {
if (__rlen < __n2)
diff --git a/libcxx/test/std/strings/basic.string/min_call_varying_size_types.pass.cpp b/libcxx/test/std/strings/basic.string/min_call_varying_size_types.pass.cpp
new file mode 100644
index 0000000000000..7128bc0313e51
--- /dev/null
+++ b/libcxx/test/std/strings/basic.string/min_call_varying_size_types.pass.cpp
@@ -0,0 +1,246 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <string>
+
+// XFAIL: FROZEN-CXX03-HEADERS-FIXME
+
+// Make sure basic_string constructors and functions operate properly for allocators with small `size_type`s.
+// Related issue: https://github.com/llvm/llvm-project/issues/125187
+
+// constexpr basic_string(
+// basic_string&& str, size_type pos, size_type n, const Allocator& a = Allocator()); // C++23
+// basic_string(const basic_string& str, size_type pos, size_type n,
+// const Allocator& a = Allocator()); // constexpr since C++20
+// basic_string& assign(const basic_string& str, size_type pos, size_type n=npos); // constexpr since C++20
+// template <class T>
+// basic_string& assign(const T& t, size_type pos, size_type n=npos); // C++17, constexpr since C++20
+// basic_string& append(const basic_string& str, size_type pos, size_type n=npos); // constexpr since C++20
+// template <class T>
+// basic_string& append(const T& t, size_type pos, size_type n=npos); // C++17, constexpr since C++20
+// basic_string& insert(size_type pos1, const basic_string& str,
+// size_type pos2, size_type n=npos); // constexpr since C++20
+// template <class T>
+// basic_string& insert(size_type pos1, const T& t, size_type pos2, size_type n=npos); // C++17, constexpr since C++20
+// basic_string& erase(size_type pos = 0, size_type n = npos); // constexpr since C++20
+// basic_string& replace(size_type pos, size_type n1, const value_type* s, size_type n2); // constexpr since C++20
+// basic_string& replace(size_type pos, size_type n1, size_type n2, value_type c); // constexpr since C++20
+// basic_string& replace(size_type pos1, size_type n1, const basic_string& str,
+// size_type pos2, size_type n2=npos); // constexpr since C++20
+// template <class T>
+// basic_string& replace(size_type pos1, size_type n1, const T& t,
+// size_type pos2, size_type n2= npos); // C++17, constexpr since C++20
+// size_type copy(value_type* s, size_type n, size_type pos = 0) const; // constexpr since C++20
+// template <class T>
+// int compare(const T& t) const noexcept; // C++17, constexpr since C++20
+// int compare(size_type pos1, size_type n1, const value_type* s, size_type n2) const; // constexpr since C++20
+
+#include <cassert>
+#include <cstdint>
+#include <string>
+#include <string_view>
+
+#include "sized_allocator.h"
+#include "test_macros.h"
+
+template <class SizeT, class DiffT, class CharT = char, class Traits = std::char_traits<CharT> >
+TEST_CONSTEXPR_CXX20 void test_with_custom_size_type() {
+ using Alloc = sized_allocator<CharT, SizeT, DiffT>;
+ using string = std::basic_string<CharT, Traits, Alloc>;
+ string s = "hello world";
+
+ // The following tests validate all possible calls to std::min within <basic_string>
+ { // basic_string(const basic_string& str, size_type pos, size_type n, const Allocator& a = Allocator())
+ assert(string(s, 0, 5) == "hello");
+ assert(string(s, 0, 5, Alloc(3)) == "hello");
+ assert(string(s, 6, 5) == "world");
+ assert(string(s, 6, 5, Alloc(3)) == "world");
+ assert(string(s, 6, 100) == "world");
+ assert(string(s, 6, 100, Alloc(3)) == "world");
+ }
+#if TEST_STD_VER >= 23
+ { // constexpr basic_string(basic_string&& str, size_type pos, size_type n, const Allocator& a = Allocator());
+ assert(string(string(s), 0, 5) == "hello");
+ assert(string(string(s), 0, 5, Alloc(3)) == "hello");
+ assert(string(string(s), 6, 5) == "world");
+ assert(string(string(s), 6, 5, Alloc(3)) == "world");
+ assert(string(string(s), 6, 100) == "world");
+ assert(string(string(s), 6, 100, Alloc(3)) == "world");
+ }
+#endif
+ { // basic_string& assign(const basic_string& str, size_type pos, size_type n=npos)
+ string s1 = s;
+ string s2 = "cplusplus";
+ s1.assign(s2, 0, 5);
+ assert(s1 == "cplus");
+ s1.assign(s2, 0, 9);
+ assert(s1 == "cplusplus");
+ s1.assign(s2, 5);
+ assert(s1 == "plus");
+ s1.assign(s2, 0, 100);
+ assert(s1 == "cplusplus");
+ s1.assign(s2, 4);
+ assert(s1 == "splus");
+ s1.assign(s2, 0);
+ assert(s1 == "cplusplus");
+ }
+#if TEST_STD_VER >= 17
+ { // template <class T> basic_string& assign(const T& t, size_type pos, size_type n=npos)
+ std::string_view sv = "cplusplus";
+ string s1 = s;
+ s1.assign(sv, 0, 5);
+ assert(s1 == "cplus");
+ s1.assign(sv, 0, 9);
+ assert(s1 == "cplusplus");
+ s1.assign(sv, 5);
+ assert(s1 == "plus");
+ s1.assign(sv, 0, 100);
+ assert(s1 == "cplusplus");
+ s1.assign(sv, 4);
+ assert(s1 == "splus");
+ s1.assign(sv, 0);
+ assert(s1 == "cplusplus");
+ }
+#endif
+ { // basic_string& append(const basic_string& str, size_type pos, size_type n=npos)
+ string s1 = s;
+ string s2 = " of cplusplus";
+ s1.append(s2, 0, 5);
+ assert(s1 == "hello world of c");
+ s1 = s;
+ s1.append(s2, 0, 100);
+ assert(s1 == "hello world of cplusplus");
+ s1 = s;
+ s1.append(s2, 0);
+ assert(s1 == "hello world of cplusplus");
+ }
+#if TEST_STD_VER >= 17
+ { // template <class T> basic_string& append(const T& t, size_type pos, size_type n=npos)
+ string s1 = s;
+ std::string_view sv = " of cplusplus";
+ s1.append(sv, 0, 5);
+ assert(s1 == "hello world of c");
+ s1 = s;
+ s1.append(sv, 0, 100);
+ assert(s1 == "hello world of cplusplus");
+ s1 = s;
+ s1.append(sv, 0);
+ assert(s1 == "hello world of cplusplus");
+ }
+#endif
+ { // basic_string& insert(size_type pos1, const basic_string& str, size_type pos2, size_type n=npos)
+ string s1 = s;
+ string s2 = " cplusplus";
+ s1.insert(5, s2, 0, 2);
+ assert(s1 == "hello c world");
+ s1 = s;
+ s1.insert(5, s2, 0, 100);
+ assert(s1 == "hello cplusplus world");
+ s1 = s;
+ s1.insert(5, s2, 0);
+ assert(s1 == "hello cplusplus world");
+ }
+#if TEST_STD_VER >= 17
+ { // template <class T> basic_string& insert(size_type pos1, const T& t, size_type pos2, size_type n=npos)
+ string s1 = s;
+ std::string_view sv = " cplusplus";
+ s1.insert(5, sv, 0, 2);
+ assert(s1 == "hello c world");
+ s1 = s;
+ s1.insert(5, sv, 0, 100);
+ assert(s1 == "hello cplusplus world");
+ s1 = s;
+ s1.insert(5, sv, 0);
+ assert(s1 == "hello cplusplus world");
+ }
+#endif
+ { // basic_string& erase(size_type pos = 0, size_type n = npos)
+ string s1 = s;
+ assert(s1.erase(5, 100) == "hello");
+ s1 = s;
+ assert(s1.erase(5) == "hello");
+ assert(s1.erase().empty());
+ }
+ { // basic_string& replace(size_type pos, size_type n1, const value_type* s, size_type n2)
+ string s1 = s;
+ const char* s2 = "cpluscplus";
+ assert(s1.replace(6, 5, s2, 1) == "hello c");
+ assert(s1.replace(6, 1, s2, 10) == "hello cpluscplus");
+ }
+ { // basic_string& replace(size_type pos, size_type n1, size_type n2, value_type c)
+ string s1 = s;
+ assert(s1.replace(5, 6, 2, 'o') == "hellooo");
+ assert(s1.replace(5, 2, 0, 'o') == "hello");
+ }
+ { // basic_string& replace(size_type pos1, size_type n1, const basic_string& str, size_type pos2, size_type n2=npos)
+ string s1 = s;
+ string s2 = "cplusplus";
+ assert(s1.replace(6, 5, s2, 0, 1) == "hello c");
+ assert(s1.replace(7, 0, s2, 1, 9) == "hello cplusplus");
+ s1 = s;
+ assert(s1.replace(6, 5, s2, 0, 100) == "hello cplusplus");
+ s1 = s;
+ assert(s1.replace(6, 5, s2, 0) == "hello cplusplus");
+ }
+#if TEST_STD_VER >= 17
+ { // template <class T> basic_string& replace(size_type pos1, size_type n1, const T& t, size_type pos2, size_type n2= npos)
+ string s1 = s;
+ std::string_view sv = "cplusplus";
+ assert(s1.replace(6, 5, sv, 0, 1) == "hello c");
+ assert(s1.replace(7, 0, sv, 1, 9) == "hello cplusplus");
+ s1 = s;
+ assert(s1.replace(6, 5, sv, 0, 100) == "hello cplusplus");
+ s1 = s;
+ assert(s1.replace(6, 5, sv, 0) == "hello cplusplus");
+ }
+#endif
+ { // size_type copy(value_type* s, size_type n, size_type pos = 0) const
+ string s1 = s;
+ char bar[100] = {};
+ s1.copy(bar, s1.size());
+ assert(s1 == bar);
+ }
+ { // int compare(size_type pos1, size_type n1, const value_type* s, size_type n2) const
+ string s1 = s;
+ assert(s1.compare(0, 11, "hello world", 11) == 0);
+ assert(s1.compare(0, 11, "hello", 11) > 0);
+ assert(s1.compare(0, 11, "hello world C++", 100) < 0);
+ }
+#if TEST_STD_VER >= 17
+ { // template <class T> int compare(const T& t) const noexcept;
+ using std::operator""sv;
+ string s1 = s;
+ assert(s1.compare("hello world"sv) == 0);
+ assert(s1.compare("hello"sv) > 0);
+ assert(s1.compare("hello world C++"sv) < 0);
+ }
+#endif
+}
+
+TEST_CONSTEXPR_CXX20 bool test() {
+ test_with_custom_size_type<unsigned char, signed char>();
+ test_with_custom_size_type<unsigned short, short>();
+ test_with_custom_size_type<unsigned short, short>();
+ test_with_custom_size_type<unsigned, int>();
+ test_with_custom_size_type<std::size_t, std::ptrdiff_t>();
+ test_with_custom_size_type<std::uint8_t, std::int8_t>();
+ test_with_custom_size_type<std::uint16_t, std::int16_t>();
+ test_with_custom_size_type<std::uint32_t, std::int32_t>();
+ test_with_custom_size_type<std::uint64_t, std::int64_t>();
+
+ return true;
+}
+
+int main(int, char**) {
+ test();
+#if TEST_STD_VER > 17
+ static_assert(test());
+#endif
+
+ return 0;
+}
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation of
basic_stringrelies onstd::minto calculate the effective character length to be processed in various constructors and functions withinbasic_string. A typical invocation looks like this:std::min(__n, __sz - __pos);where
__n,__sz, and__posare ofsize_type(a member typedef forallocator_traits<allocator_type>::size_type). This works correctly when we use the standard allocators (std::allocator<CharT>). However, if a customer allocator is provided wheresize_typeis smaller thanint, this call is broken due to an ambiguity. Specifically, the evaluation of__sz - __posgets promoted toint. This leads to mismatched argument types (size_typeandint) forstd::min, which triggers an ambiguous call error.This issue is widespread across
basic_string, affecting 16 operations (See [1] below). Fixing it is important to ensure robust functionality, particularly when custom allocators are used.The fix is straightforward: explicitly specify the template type argument for
std::min. For example:Closes #125187.
[1]
basic_stringoperations that currently involve ambiguous calls tostd::min(this includes all operations that accepts a__posparameter):