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

Thu Nov 25 08:37: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:
> jloser wrote:
> > Quuxplusone wrote:
> > > 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)?
> > @Quuxplusone Successful nerd snipe then. :)
> > 
> > I do agree that this is where the Standard should be heading - with `string_view` and other class templates that are mandated to have the operators as non-member functions. I'm not aware of any papers in flight, but I'd be happy to work in that area if you're interested. Prior implementation experience never hurts. In practice, making these `string_view` operators hidden friends shouldn't break much code out in the wild I imagine.
> > 
> > As for the `UNSUPPORTED` for the purposes of this PR, is that possible? The problem is happening when the windows-dll build is building libc++ as I understand it - not when running a particular test.  Would you just XFAIL all of my new tests and then it won't even compile them for windows-dll build? I was under the impression it would compile them still but they'd be allowed to fail. Or am I missing something?
> Ah, if the failure is //while building the stuff in `src/`// then UNSUPPORTED/XFAIL won't be sufficient. I thought the failure was specifically on one of your new tests, like, if you write
> ```
> (void)(sv == "hello");
> (void)("hello" == sv);
> ```
> in a new test, it'll explode. But //that// was //always// the case; such a test would blow up MSVC today, if we had such a test, which we don't.
> 
> So I'm still confused about what you're saying has changed in this PR. The "sufficient overloads" are poison to MSVC both before and after this patch; nothing has changed that I can see.
> 
> Btw, Microsoft STL works around this by giving the "additional sufficient overloads" additional defaulted template parameters: https://github.com/microsoft/STL/blob/main/stl/inc/xstring#L1854  and I suppose we can work around it the same way.
I added the default template parameters to the "sufficient overloads" so msvc will be happy with this PR (don't love it, but it's the simplest change to make likely).

I don't yet fully understand what has changed in this PR to invoke the `operator!=` problem on msvc. I *think* somewhere in filesystem in a Windows only codepath, we must be invoking two different `string_view::operator!=` - one with `bsv/bsv` as its arguments and another with `bsv/type convertible to bsv` that would invoke the other sufficient overload and cause the mangling issue. I've tried to only invoke the `bsv/bsv` `std::string_view::operator!=`, but it seems we haven't caught all the cases. I only intentionally touched the places in `path` that were causing atomic constraints caching problems; it just happened to be that the one in `operator/=` for windows happens to cause the mangled symbol issues AFAICT. I think it must be here or in that neighborhood since I split up the path comparison operator changes in https://reviews.llvm.org/D114570 and CI passed fine on that.

I'm not terrible interested in spending a lot of time on it given it's annoying to figure out locally without access to windows. I think it won't matter soon since if I understand correctly, you volunteered (thanks!) to:

  - Expand on the sufficient overloads tests if needed when rewriting them to be hidden friends or when working on `operator<=>` for `string_view`.

We also already worked around the issue by adding default template arguments. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161