[libcxx-commits] [PATCH] D142843: [libc++] Addresses LWG3358

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/span:241-246
+      // [span.cons]/10
+      // Throws: When and what last - first throws.
+      static_cast<void>(__last - __first);
       _LIBCPP_ASSERT((__last - __first >= 0), "invalid range in span's constructor (iterator, sentinel)");
       _LIBCPP_ASSERT(__last - __first == _Extent,
                      "invalid range in span's constructor (iterator, sentinel): last - first != extent");
----------------



================
Comment at: libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp:123
+  std::array a{42};
+  TEST_VALIDATE_EXCEPTION((std::span<int>{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}}),    //
+                          int,                                                                                 //
----------------
Mordante wrote:
> ldionne wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > philnik wrote:
> > > > > Why not just a try/catch like we normally do?
> > > > I added new macros to make this simpler in D142808, this is based on the macros in rapid-cxx-test.h which we have, but they seem quite unknown.
> > > TBH I don't think this is a readability improvement. This just makes it harder to see what's going on.
> > Yeah, I must agree that seeing it in use here it isn't super easy to read. This would be the equivalent code right?
> > 
> > 
> > ```
> > try {
> >   std::span<int> s{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}});
> >   assert(false);
> > } catch (int i) {
> >   assert(i == 42);
> > }
> > ```
> > 
> > I think the part that gets confusing here is probably the fact that we have a predicate that tests the content of the exception. I wonder how useful that is in practice. Otherwise, this could look like `TEST_THROWS(int, []{ statements... })`, which IMO is easy to read.
> > Yeah, I must agree that seeing it in use here it isn't super easy to read. This would be the equivalent code right?
> 
> Any suggestions how to make this macro more readable?
>  
> > ```
> > try {
> >   std::span<int> s{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}});
> >   assert(false);
> > } catch (int i) {
> >   assert(i == 42);
> > }
> > ```
> 
> Not this is not equivalent, if `"hello world"` is thrown, the test passes. There needs to be a `catch (...)`.
> 
> > I think the part that gets confusing here is probably the fact that we have a predicate that tests the content of the exception. I wonder how useful that is in practice. Otherwise, this could look like `TEST_THROWS(int, []{ statements... })`, which IMO is easy to read.
> 
> We indeed have a macro for that case too.
> 
> For format tests I find it very useful to test the contents of `what()`, there I use it to validate the expected error condition is triggered. The format library throws at a lot of places and always the same type. On occasion I've made errors in the format string which resulted in an exception. There it was easy to validate the test was wrong.
> 
Just thinking out loud:

```
#ifndef TEST_HAS_NO_EXCEPTIONS
assert(42 == thrown_exception<int>([]{
  std::span<int> s{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}};
  (void)s;
}));
#endif
```

I think there is more benefit to this macro when there are multiple tests back-to-back that use it. Then you do the mental work of figuring out what the macro does once, and you read it mechanically multiple times. In this case here though, there's only two assertions and none elsewhere in the file so the benefit feels less clear to me.

I don't want to block this patch on this though. Feel free to land but let's give some thought to how we can make `TEST_VALIDATE_EXCEPTION` better generally speaking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142843