[libc++][NFC] Simplify the implementation of string and string_views operator== #117184

philnik777 · 2024-11-21T16:29:36Z

No description provided.

llvmbot · 2024-11-22T23:09:18Z

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

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

2 Files Affected:

(modified) libcxx/include/string (+8-32)
(modified) libcxx/include/string_view (+12-28)

diff --git a/libcxx/include/string b/libcxx/include/string
index bf7fc3c37ecd7a..e8b543355665c3 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1879,11 +1879,6 @@ public:
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __clear_and_shrink() _NOEXCEPT;
 
 private:
-  template <class _Alloc>
-  inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 bool friend
-  operator==(const basic_string<char, char_traits<char>, _Alloc>& __lhs,
-             const basic_string<char, char_traits<char>, _Alloc>& __rhs) _NOEXCEPT;
-
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __shrink_or_extend(size_type __target_capacity);
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS bool
@@ -3840,36 +3835,9 @@ template <class _CharT, class _Traits, class _Allocator>
 inline _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool
 operator==(const basic_string<_CharT, _Traits, _Allocator>& __lhs,
            const basic_string<_CharT, _Traits, _Allocator>& __rhs) _NOEXCEPT {
-#if _LIBCPP_STD_VER >= 20
-  return basic_string_view<_CharT, _Traits>(__lhs) == basic_string_view<_CharT, _Traits>(__rhs);
-#else
   size_t __lhs_sz = __lhs.size();
   return __lhs_sz == __rhs.size() && _Traits::compare(__lhs.data(), __rhs.data(), __lhs_sz) == 0;
-#endif
-}
-
-template <class _Allocator>
-inline _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool
-operator==(const basic_string<char, char_traits<char>, _Allocator>& __lhs,
-           const basic_string<char, char_traits<char>, _Allocator>& __rhs) _NOEXCEPT {
-  size_t __sz = __lhs.size();
-  if (__sz != __rhs.size())
-    return false;
-  return char_traits<char>::compare(__lhs.data(), __rhs.data(), __sz) == 0;
-}
-
-#if _LIBCPP_STD_VER <= 17
-template <class _CharT, class _Traits, class _Allocator>
-inline _LIBCPP_HIDE_FROM_ABI bool
-operator==(const _CharT* __lhs, const basic_string<_CharT, _Traits, _Allocator>& __rhs) _NOEXCEPT {
-  typedef basic_string<_CharT, _Traits, _Allocator> _String;
-  _LIBCPP_ASSERT_NON_NULL(__lhs != nullptr, "operator==(char*, basic_string): received nullptr");
-  size_t __lhs_len = _Traits::length(__lhs);
-  if (__lhs_len != __rhs.size())
-    return false;
-  return __rhs.compare(0, _String::npos, __lhs, __lhs_len) == 0;
 }
-#endif // _LIBCPP_STD_VER <= 17
 
 template <class _CharT, class _Traits, class _Allocator>
 inline _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool
@@ -3888,6 +3856,14 @@ operator==(const basic_string<_CharT, _Traits, _Allocator>& __lhs, const _CharT*
   return __lhs.compare(0, _String::npos, __rhs, __rhs_len) == 0;
 }
 
+#if _LIBCPP_STD_VER <= 17
+template <class _CharT, class _Traits, class _Allocator>
+inline _LIBCPP_HIDE_FROM_ABI bool
+operator==(const _CharT* __lhs, const basic_string<_CharT, _Traits, _Allocator>& __rhs) _NOEXCEPT {
+  return __rhs == __lhs;
+}
+#endif // _LIBCPP_STD_VER <= 17
+
 #if _LIBCPP_STD_VER >= 20
 
 template <class _CharT, class _Traits, class _Allocator>
diff --git a/libcxx/include/string_view b/libcxx/include/string_view
index 6e05844b72cadf..b4bd061419b343 100644
--- a/libcxx/include/string_view
+++ b/libcxx/include/string_view
@@ -720,16 +720,19 @@ basic_string_view(_Range) -> basic_string_view<ranges::range_value_t<_Range>>;
 
 // [string.view.comparison]
 
-#if _LIBCPP_STD_VER >= 20
-
-template <class _CharT, class _Traits>
-_LIBCPP_HIDE_FROM_ABI constexpr bool operator==(basic_string_view<_CharT, _Traits> __lhs,
-                                                type_identity_t<basic_string_view<_CharT, _Traits>> __rhs) noexcept {
+// The dummy default template parameters are used to work around a MSVC issue with mangling, see VSO-409326 for details.
+// This applies to the other sufficient overloads below for the other comparison operators.
+template <class _CharT, class _Traits, int = 1>
+_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI bool
+operator==(basic_string_view<_CharT, _Traits> __lhs,
+           __type_identity_t<basic_string_view<_CharT, _Traits> > __rhs) _NOEXCEPT {
   if (__lhs.size() != __rhs.size())
     return false;
   return __lhs.compare(__rhs) == 0;
 }
 
+#if _LIBCPP_STD_VER >= 20
+
 template <class _CharT, class _Traits>
 _LIBCPP_HIDE_FROM_ABI constexpr auto operator<=>(basic_string_view<_CharT, _Traits> __lhs,
                                                  type_identity_t<basic_string_view<_CharT, _Traits>> __rhs) noexcept {
@@ -755,51 +758,32 @@ operator==(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _
   return __lhs.compare(__rhs) == 0;
 }
 
-// The dummy default template parameters are used to work around a MSVC issue with mangling, see VSO-409326 for details.
-// This applies to the other sufficient overloads below for the other comparison operators.
-template <class _CharT, class _Traits, int = 1>
-_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI bool
-operator==(basic_string_view<_CharT, _Traits> __lhs,
-           __type_identity_t<basic_string_view<_CharT, _Traits> > __rhs) _NOEXCEPT {
-  if (__lhs.size() != __rhs.size())
-    return false;
-  return __lhs.compare(__rhs) == 0;
-}
-
 template <class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI bool
 operator==(__type_identity_t<basic_string_view<_CharT, _Traits> > __lhs,
            basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT {
-  if (__lhs.size() != __rhs.size())
-    return false;
-  return __lhs.compare(__rhs) == 0;
+  return __lhs == __rhs;
 }
 
 // operator !=
 template <class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI bool
 operator!=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT {
-  if (__lhs.size() != __rhs.size())
-    return true;
-  return __lhs.compare(__rhs) != 0;
+  return !(__lhs == __rhs);
 }
 
 template <class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI bool
 operator!=(basic_string_view<_CharT, _Traits> __lhs,
            __type_identity_t<basic_string_view<_CharT, _Traits> > __rhs) _NOEXCEPT {
-  if (__lhs.size() != __rhs.size())
-    return true;
-  return __lhs.compare(__rhs) != 0;
+  return !(__lhs == __rhs);
 }
 
 template <class _CharT, class _Traits, int = 2>
 _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI bool
 operator!=(__type_identity_t<basic_string_view<_CharT, _Traits> > __lhs,
            basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT {
-  if (__lhs.size() != __rhs.size())
-    return true;
-  return __lhs.compare(__rhs) != 0;
+  return !(__lhs == __rhs);
 }
 
 // operator <

ldionne

Makes sense to me but I have some questions.

libcxx/include/string

ldionne · 2024-11-26T19:43:49Z

libcxx/include/string_view

-  if (__lhs.size() != __rhs.size())
-    return false;
-  return __lhs.compare(__rhs) == 0;
+  return __lhs == __rhs;


Is this not recursive? What did I miss?

This calls the operator==(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs).

I must be missing the point of the __type_identity argument here. It seems to be central to my misunderstanding.

The __type_identity is to allow types that are convertible to string_view to be compared to a string_view.

But how are these overloads not ambiguous with the one that takes two basic_string_views when you pass actual string views (that match both overloads)?

Gentle ping.

My understanding is that if you have more exact matches for the template parameter that is preferred over an argument that happens to have the same type. Consider e.g.

template <class T, class U> void func(T, U); template <class T> void func(T, T);

Here the second overload is preferred, since it requires that the two arguments match exactly. I'm not 100% sure what the rule here is though.

github-actions · 2024-12-10T20:11:14Z

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

…operator==

philnik777 marked this pull request as ready for review November 22, 2024 23:07

philnik777 requested a review from a team as a code owner November 22, 2024 23:07

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 22, 2024

ldionne reviewed Nov 26, 2024

View reviewed changes

ldionne approved these changes Dec 10, 2024

View reviewed changes

philnik777 force-pushed the simplify_string_eq branch from f8d5576 to ecdc97b Compare December 10, 2024 20:07

[libc++][NFC] Simplify the implementation of string and string_views …

2c4c79c

…operator==

philnik777 force-pushed the simplify_string_eq branch from ecdc97b to 2c4c79c Compare December 12, 2024 16:24

philnik777 merged commit 5fd385b into llvm:main Dec 13, 2024
63 checks passed

philnik777 deleted the simplify_string_eq branch December 13, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++][NFC] Simplify the implementation of string and string_views operator== #117184

[libc++][NFC] Simplify the implementation of string and string_views operator== #117184

Uh oh!

philnik777 commented Nov 21, 2024

Uh oh!

llvmbot commented Nov 22, 2024

Uh oh!

ldionne left a comment

Uh oh!

Uh oh!

ldionne Nov 26, 2024

Uh oh!

philnik777 Nov 26, 2024

Uh oh!

ldionne Nov 26, 2024

Uh oh!

philnik777 Nov 26, 2024

Uh oh!

ldionne Nov 26, 2024

Uh oh!

ldionne Dec 9, 2024

Uh oh!

philnik777 Dec 10, 2024

Uh oh!

github-actions bot commented Dec 10, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

[libc++][NFC] Simplify the implementation of string and string_views operator== #117184

[libc++][NFC] Simplify the implementation of string and string_views operator== #117184

Uh oh!

Conversation

philnik777 commented Nov 21, 2024

Uh oh!

llvmbot commented Nov 22, 2024

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldionne Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

philnik777 Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

ldionne Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

philnik777 Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

ldionne Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

ldionne Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

philnik777 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 10, 2024 •

edited

Loading