[libcxx-commits] [PATCH] D113161: [libc++] Implement P1989R2: range constructor for string_view

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/string_view:761-789
 // operator !=
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator!=(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
 {
     if ( __lhs.size() != __rhs.size())
         return true;
----------------
IIUC, the three overloads (new lines 764, 773, 783) are intended to accomplish https://en.cppreference.com/w/cpp/string/basic_string_view/operator_cmp 's wording "The implementation shall provide sufficient additional constexpr and noexcept overloads of these functions so that a basic_string_view<CharT,Traits> object sv may be compared to another object t with an implicit conversion to basic_string_view<CharT,Traits>, with semantics identical to comparing sv and basic_string_view<CharT,Traits>(t)."

(1) I don't see how anything in this PR should have affected these overloads at all. I believe it makes sense to me that MSVC wouldn't like these overloads, because MSVC (non-conformingly) doesn't let the spelling of a type affect its mangling — `__type_identity_t<BSV>` and `BSV` are going to get the same mangling, which is going to be bad news for MSVC. //But//, this was already the case prior to your patch, when the parameters had types `common_type<BSV>::type` and `BSV`. MSVC should //already// have been complaining. If it wasn't, then either I don't understand something, //or// maybe there was just a hole in our test coverage. (Basically we need a test case that instantiates both `sv == "hello"` and `"hello" == sv` in the same TU; my hypothesis is that MSVC will barf at that, even before this patch.)
Evidence in favor: https://godbolt.org/z/1MMaK9P33

(2) If we believe it's worthwhile to depart from the status quo and start messing with these overloads, then I would like to see us just take the leap and start using hidden friends for these operators. Hidden friend idiom solves the problem instantly AFAIK.

(3) We certainly should implement `<=>` for string_view, too, as long as we're messing with this code; but I hypothesize that `<=>` will //not// solve any problem here: `<=>` will have the same problems as `==` does today, and `==` will continue having those problems anyway in modes prior to `-std=c++20`. So I encourage Someone to tackle that, but with the understanding that it's not going to fix anything. ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113161/new/

https://reviews.llvm.org/D113161