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

Wed Nov 24 17:35:24 PST 2021

Quuxplusone 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;
----------------
jloser wrote:
> jloser wrote:
> > 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. 
> @Quuxplusone making the comparison operators hidden friends would make our conforming implementation non-conforming. They are required to be non-member functions according to the standard.
> 
> I spoke with @davidstone just a bit ago and we think that a paper could be written to make these hidden friends instead. But right now, it would be non-conforming for `libc++` to turn them into hidden friends as far as I understand it.
> 
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1614r2.html#was-well-formed-now-ill-formed
> making the comparison operators hidden friends would make our implementation non-conforming. They are required to be non-member functions [...] right now, it would be non-conforming for libc++ to turn them into hidden friends as far as I understand it.

I believe that's all correct; but, IMO it's still worth taking the plunge. It's clearly where the Standard is //going// to wind up, in 3 or 6 or 9 or 12 years, and part (only part!) of what's holding it back is that implementors aren't providing "existing practice" to standardize. The quote from p1614r2 seems to me like evidence supporting "Just hidden-friend everything already": Barry seems to be saying that LEWG //knew// that hidden-friending `operator<=>` would change behavior in this obscure corner case, and they decided that it was totally worth it — p1614r2 was adopted, right? even with this breakage explicitly called out in a dedicated section of the paper.

I think I'm getting nerdsniped into both implementing `<=>` for `string_view` and refactoring these other operators. I'll make a PR. :) For purposes of D113161, the situation seems to be "Pre-existing incompatibility with MSVC means that some of @jloser's new tests will fail on msvc (only prior to C++20?)," and the solution is to `UNSUPPORTED` those new tests with a comment in the commit message. And then maybe one of my later PRs will remove that `UNSUPPORTED`ness. Does that make sense as a way forward (while keeping this PR simple)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161