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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
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,                                                                                 //
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142843