[libcxx] Optimize away unneeded length calculation in basic_string::compare(const char*)

[EricWF]

Summary

This patch optimizes basic_string::compare to use strcmp when the default char_traits has been given.
See PR19900 for more information. https://llvm.org/bugs/show_bug.cgi?id=19900

[mclow.lists]

I think I'd rather see the call to strcmp and wcscmp in the char_traits class.

[bkramer]

Won't this do the wrong thing for embedded '\0' in a std::string?

std::string("hello\0world", 11).compare("hello")

should not return 0.

[EricWF]

In D12355#233477, @bkramer wrote:

Won't this do the wrong thing for embedded '\0' in a std::string?

std::string("hello\0world", 11).compare("hello")

should not return 0.

Woops. Yep that seems correct but it will sure hamper the possibility for optimization.

[mclow.lists]

In D12355#233477, @bkramer wrote:

Won't this do the wrong thing for embedded '\0' in a std::string?

std::string("hello\0world", 11).compare("hello")

should not return 0.

Good point; I think that pretty much kills this proposed change.

[EricWF]

Instead of optimizing away the length calculation, this patch still performs it but uses the calculated length to return early if the two strings differ in length.

[EricWF]

Fix debug assertion.

[mclow.lists]

include/string

3801

If the lengths are the same (and they are), you don't need the min of them

3802

Also, you should not call traits::compare here. You should call __rhs.compare(0, npos, __lhs, __lhs_len)

which is what __rhs.compare(__lhs) does.

[EricWF]

include/string

3802

I actually don't want to invoke compare because it has different semantics and does unneeded work. compare first does a comparison using traits::compare and then does a comparison using length. We have already compared based on length and we don't need to do any of the extra work that compare(...) does. I think calling __rhs.compare(0, npos, __lhs, __lhs_len) would be a pessimization.

[EricWF]

Remove unnecessary call to std::min.

[EricWF]

Call basic_string::compare as requested by @mclow.lists.

[mclow.lists]

The first change LGTM. The second one needs to match it.

include/string

3814

You'll want to do the same as above here.

include/string

 operator==(const _CharT* __lhs,
            const basic_string<_CharT, _Traits, _Allocator>& __rhs) _NOEXCEPT
 {
-    return __rhs.compare(__lhs) == 0;
+    _LIBCPP_ASSERT(__lhs != nullptr, "operator==(basic_string, basic_string): received nullptr");
+    size_t __lhs_len = _Traits::length(__lhs);
+    if (__lhs_len != __rhs.size()) return false;
+    return _Traits::compare(__rhs.data(), __lhs, _VSTD::min(__lhs_len, __rhs.size())) == 0;
 }
 
 template<class _CharT, class _Traits, class _Allocator>

 operator==(const basic_string<_CharT,_Traits,_Allocator>& __lhs,
            const _CharT* __rhs) _NOEXCEPT
 {
-    return __lhs.compare(__rhs) == 0;
+    _LIBCPP_ASSERT(__lhs != nullptr, "operator==(basic_string, basic_string): received nullptr");
+    size_t __rhs_len = _Traits::length(__rhs);
+    if (__rhs_len != __lhs.size()) return false;
+    return _Traits::compare(__lhs.data(), __rhs, _VSTD::min(__rhs_len, __lhs.size())) == 0;
 }
 
 // operator!=