These macros are intended to replace the macros in rapid-cxx-test.h.
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG3476b56f0c78: [libc++][test] Adds more generic test macros.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/test/support/assert_macros.h | ||
---|---|---|
37–38 | I would use abort instead of exit -- some platforms can provide abort but can't provide exit, because exit has to be able to terminate the program successfully, which doesn't make sense on some platforms. abort() should be available on more platforms than exit. | |
41 | It would also be nice to avoid mentioning std::cerr here. Would it make sense to pass a file descriptor like stderr to the functor being called so it can use std::fprintf(descriptor, ...) directly? | |
56–58 | I would separate the exit out of the logging function. It would look like this ^. | |
58–62 | ||
67–70 | ||
82 | Why not define this conditionally to nothing or to test_require? That would allow getting rid of test_libcpp_require above. | |
90 | ||
98 | Please fully qualify throughout. | |
112 | I feel that the filesystem order of arguments is clearer: exception type, exception predicate and finally the expression. Not a super strong opinion though. | |
120 | To avoid using a non-reserved name? |
Thanks for the review!
libcxx/test/support/assert_macros.h | ||
---|---|---|
41 | I don't think it makes sense to provide a descriptor. This should only log messages when an error occurs. Using printf(stderr, ...) instead of std::cerr, probably makes sense. | |
82 | Good point! | |
120 | I wouldn't mind using exception, but we shouldn't use reserved names in our tests. This would break the test when a standard library uses __exception. |
libcxx/docs/TestingLibcxx.rst | ||
---|---|---|
153 ↗ | (On Diff #495071) | |
156–158 ↗ | (On Diff #495071) | |
159 ↗ | (On Diff #495071) | Etc.. It's less confusing for people if we consistently use paths from the root of the monorepo. |
162–163 ↗ | (On Diff #495071) | |
164–169 ↗ | (On Diff #495071) | |
170–172 ↗ | (On Diff #495071) | I would avoid documenting that, since it's only useful for frequent contributors and they pretty much already know where to find those. I am a bit worried about some of this information becoming out of date. |
228 ↗ | (On Diff #495071) | |
231 ↗ | (On Diff #495071) | |
279 ↗ | (On Diff #495071) | |
282 ↗ | (On Diff #495071) | |
303–309 ↗ | (On Diff #495071) | This looks like a footgun to me, I would not provide that. Hiding this sort of mechanism behind a macro seems like a bad idea to me. I would instead return manually and add a comment. This isn't great, but the real solution would be to have a way to integrate this into lit so that lit can show us the test as being UNSUPPORTED based on a runtime condition, which is a much more involved thing to do. |
libcxx/test/support/assert_macros.h | ||
37–38 | I don't think you actually need the string overload. That would allow you to drop the header dependency. |
Thanks for the review. I will look at the other comments after testing locally.
libcxx/docs/TestingLibcxx.rst | ||
---|---|---|
170–172 ↗ | (On Diff #495071) | I'm a bit on the fence. It's indeed most useful for frequent contributors, but not documented makes it harder for new contributors to become frequent contributors. Since I already have a link to libcxx/utils/libcxx/test/format.py it's quite easy to find this directory. So I'll remove it. |
303–309 ↗ | (On Diff #495071) | As mentioned in our 1:1, this is added as compatibility with the existing rapid tests. As agreed I will remove this and update the test using this macro to use that approach. |
libcxx/test/support/assert_macros.h | ||
---|---|---|
37–38 | Good catch, you're correct. I also tested the follow-up patches and they keep working when removing the std::string overloads. |
LGTM, but let's take a look at TEST_VALIDATE_EXCEPTION separately to see if we really want to keep it, and if so, under what form. This is good to commit, we can address it separately.
I would use abort instead of exit -- some platforms can provide abort but can't provide exit, because exit has to be able to terminate the program successfully, which doesn't make sense on some platforms. abort() should be available on more platforms than exit.