Hoist assert from the device code. #1157
Conversation
SYCL standard doesn't require supporting C assert() function in device code. Move checks from the device code to the host code.
| test_move::evaluate(ptr); | ||
| ptr += test_move::result_count; | ||
| assert(static_cast<std::ptrdiff_t>(result_count) == ptr - results.data()); | ||
| assert(*ptr); |
There was a problem hiding this comment.
Is this supposed to be a property the SYCL implementation needs to uphold, in which case this should be:
| assert(*ptr); | |
| CHECK(*ptr); |
, or is it an invariant that if broken would indicate a programming bug in the test itself? (In which case assert is fine).
Using assert for testing would be wrong because it is disabled in Release builds!
There was a problem hiding this comment.
I assume the later.
This seems to be a common pattern in common reference semantics checks -
SYCL-CTS/tests/common/semantics_reference.h
Lines 68 to 70 in b7c8385
There was a problem hiding this comment.
Perhaps using assert() was a bad design but it was before using Catch2.
Probably nowadays using CHECK() is better.
There was a problem hiding this comment.
Both are fine if used properly. @Maetveis provided a good justification for using assert in his comment.
| test_move::evaluate(ptr); | ||
| ptr += test_move::result_count; | ||
| assert(static_cast<std::ptrdiff_t>(result_count) == ptr - results.data()); | ||
| assert(*ptr); |
There was a problem hiding this comment.
Perhaps using assert() was a bad design but it was before using Catch2.
Probably nowadays using CHECK() is better.
|
@tomdeakin, can we merge this PR? |
|
WG approved to merge. |
4 participants
SYCL standard doesn't require supporting C assert() function in device
code. Move checks from the device code to the host code.