[libcxx-commits] [PATCH] D111925: [libc++][NFC] Mark LWG3573 as complete

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 5 inline comments as done.
jloser added a comment.

In D111925#3068330 <https://reviews.llvm.org/D111925#3068330>, @Mordante wrote:

> Thanks for addressing these LWG-issues.





================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp:54
+constexpr std::iter_difference_t<I> operator-(throwing_sized_sentinel<I>, std::input_or_output_iterator auto) {
+  throw std::runtime_error("LWG 3573");
+  return {};
----------------
Quuxplusone wrote:
> Mordante wrote:
> > It's possible to use Clang in a way it won't throw exceptions. This test fails in that context. Please guard all code you added with `#ifndef TEST_HAS_NO_EXCEPTIONS`. 
> You should know I'm going to object every time someone adds a global-namespace overloaded operator to a type that already defines its own operators. If `throwing_sized_sentinel<I>` IS-A `sized_sentinel<I>`, then its `operator-` shouldn't be doing anything different from the existing `operator-` of `sized_sentinel<I>`.
> Also, although this is a technical nitpick, I believe throwing from `operator-` in this case violates the semantic requirement in https://eel.is/c++draft/iterator.concept.sizedsentinel#2.1 , which defines what `operator-` needs to return in this case (and implicitly defines that it //does// return).
> Also, `std::input_or_output_iterator auto` should be `I`, and just generally this code is way more crufty than it could be.
> 
> I think we can get by with
> ```
> template<class CharT>
> struct ThrowingSentinel {
>   friend bool operator==(const CharT*, ThrowingSentinel) noexcept;
>   friend int operator-(const CharT *, ThrowingSentinel) noexcept;
>   friend int operator-(ThrowingSentinel, const CharT *) { throw 42; }
> };
> 
> template<class CharT>
> void test_throwing() {
>   auto val = MAKE_STRING_VIEW(CharT, "test");
>   try {
>     (void)std::basic_string_view<CharT>(val.begin(), ThrowingSentinel());
>     assert(false);
>   } catch (int i) {
>     assert(i == 42);
>   }
> }
> ~~~
>   test_throwing<char>();
> #ifndef TEST_HAS_NO_WIDE_CHARACTERS
>   test_throwing<wchar_t>();
> #endif
>   test_throwing<char8_t>();
>   test_throwing<char16_t>();
>   test_throwing<char32_t>();
> ```
> (I marked some undefined functions `noexcept` merely to try to be evil and trick the library into getting some intermediate noexcept-specification wrong. I don't //really// care about those noexcepts. Those functions are undefined because they will never be called.)
Good call, just guarded with `TEST_HAS_NO_EXCEPTIONS`.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp:54
+constexpr std::iter_difference_t<I> operator-(throwing_sized_sentinel<I>, std::input_or_output_iterator auto) {
+  throw std::runtime_error("LWG 3573");
+  return {};
----------------
jloser wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > It's possible to use Clang in a way it won't throw exceptions. This test fails in that context. Please guard all code you added with `#ifndef TEST_HAS_NO_EXCEPTIONS`. 
> > You should know I'm going to object every time someone adds a global-namespace overloaded operator to a type that already defines its own operators. If `throwing_sized_sentinel<I>` IS-A `sized_sentinel<I>`, then its `operator-` shouldn't be doing anything different from the existing `operator-` of `sized_sentinel<I>`.
> > Also, although this is a technical nitpick, I believe throwing from `operator-` in this case violates the semantic requirement in https://eel.is/c++draft/iterator.concept.sizedsentinel#2.1 , which defines what `operator-` needs to return in this case (and implicitly defines that it //does// return).
> > Also, `std::input_or_output_iterator auto` should be `I`, and just generally this code is way more crufty than it could be.
> > 
> > I think we can get by with
> > ```
> > template<class CharT>
> > struct ThrowingSentinel {
> >   friend bool operator==(const CharT*, ThrowingSentinel) noexcept;
> >   friend int operator-(const CharT *, ThrowingSentinel) noexcept;
> >   friend int operator-(ThrowingSentinel, const CharT *) { throw 42; }
> > };
> > 
> > template<class CharT>
> > void test_throwing() {
> >   auto val = MAKE_STRING_VIEW(CharT, "test");
> >   try {
> >     (void)std::basic_string_view<CharT>(val.begin(), ThrowingSentinel());
> >     assert(false);
> >   } catch (int i) {
> >     assert(i == 42);
> >   }
> > }
> > ~~~
> >   test_throwing<char>();
> > #ifndef TEST_HAS_NO_WIDE_CHARACTERS
> >   test_throwing<wchar_t>();
> > #endif
> >   test_throwing<char8_t>();
> >   test_throwing<char16_t>();
> >   test_throwing<char32_t>();
> > ```
> > (I marked some undefined functions `noexcept` merely to try to be evil and trick the library into getting some intermediate noexcept-specification wrong. I don't //really// care about those noexcepts. Those functions are undefined because they will never be called.)
> Good call, just guarded with `TEST_HAS_NO_EXCEPTIONS`.
Your suggestions are a good refactoring indeed -- just applied them. I did feel a bit bad writing that original `operator-` for `throwing_sized_sentinel`, but it did satisfy the requirements for the test. :)

As for the concept for `sized_sentinel_for`, it does indeed *need* to return `std::iter_difference_t<I>`. Note that your original writing with a return type of `int` would hard error in concept checking `sized_sentinel_for` for example. I'll wave my hands a bit on whether the return *is in fact reachable* (because in this case it's not due to the unconditional throwing, but it still "works").


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp:55
+  throw std::runtime_error("LWG 3573");
+  return {};
+}
----------------
Mordante wrote:
> Is there a reason for this return?
It originally was strictly required to avoid hard erroring during concept checking `sized_sentinel_for`. 


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp:59
+template <class CharT, class Sentinel>
+constexpr bool test_noexcept() {
+  auto val = MAKE_STRING_VIEW(CharT, "test");
----------------
Mordante wrote:
> I'm not fond of `constexpr` since throwing isn't allowed in a `constexpr` function.
Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111925