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

Wed Nov 24 14:55:53 PST 2021

jloser added inline comments.

================
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;
----------------
Quuxplusone wrote:
> 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. ;)
That's my understanding as well for the additional comparison op overloads.

(1) It's missing test coverage in `libc++` I think. One of my changes in Windows-only codepath in`path` is calling a `std::string_view::operator!=` overload and *somewhere else* we must also be calling *a different* `std::string_view::operator!=` overload which causes Windows DLL builds to complain. So, it's an issue on top-of-tree but no test provoking the behavior as I understand it. As for the mangling non-conformingness, that's unfortunate. Thanks for the Godbolt link to show me how I could iterate on the issue a bit quicker without the full CI turnaround times. :)

(2) Perhaps. In order to make progress with this patch, I can either XFAIL the windows-dll build and then fix the comparison operator overload issues. Or fix the comparison operator overloads first on trunk and then rebase this patch with it. I don't feel too strongly, but it seems more polite to fix the the comparison operator overloads first and delay this patch (despite me caring more about this patch than the windows-dll problems :)). WDYT?

(3) Shame that it won't fix this problem, but I think you're right. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161