This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Addresses LWG3358
ClosedPublic

Authored by Mordante on Jan 29 2023, 7:45 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG508b4510de12: [libc++] Addresses LWG3358
Summary
LWG3358 §[span.cons] is mistaken that to_address can throw

Since last - first has to throw tests are added to make sure this always
happens.

Depends on D142808

Diff Detail

Event Timeline

Mordante created this revision.Jan 29 2023, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2023, 7:45 AM
Mordante requested review of this revision.Jan 29 2023, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2023, 7:45 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
123

Why not just a try/catch like we normally do?

Mordante added inline comments.Feb 6 2023, 5:09 AM
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
123

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.

Mordante updated this revision to Diff 495102.Feb 6 2023, 5:59 AM

Adds missing includes.

philnik added inline comments.Feb 6 2023, 7:40 AM
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
123

TBH I don't think this is a readability improvement. This just makes it harder to see what's going on.

ldionne added inline comments.
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
123

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.

Mordante added inline comments.Feb 18 2023, 5:51 AM
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
123

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.

ldionne accepted this revision.Mar 7 2023, 9:36 AM
ldionne added inline comments.
libcxx/include/span
241–246
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
123

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.

This revision is now accepted and ready to land.Mar 7 2023, 9:36 AM
Mordante marked an inline comment as done.Mar 7 2023, 10:34 AM
Mordante added inline comments.
libcxx/include/span
241–246

I kept the comment, I prefer to plainly cite the Standard, since that is the source of truth.
I agree the other proposed changes are an improvement, so I applied them.

This revision was landed with ongoing or failed builds.Mar 7 2023, 10:36 AM
This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.