-
Notifications
You must be signed in to change notification settings - Fork 66
Made count_true a new format callable #2075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
17a3ded to
4b9f64b
Compare
DenisYaroshevskiy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still in progress?
DenisYaroshevskiy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I undestand your changes to the test a bit more, here is what i suggest.
a) We stop calling count directly in test, we start calling test_count with logical and ignore
b) inside test_count function you add extra cases for all falses and maybe ignore.mask(as{})
|
Waiting for the text |
381414b to
b6158be
Compare
49af4e0 to
de115d1
Compare
| #endif | ||
|
|
||
| // Assume an expression to be true at compile time | ||
| #if defined(EVE_NO_ASSUME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this goes in ABI but w/e. @jfalcou
test/unit/module/core/count_true.cpp
Outdated
| for (std::ptrdiff_t i = 0; i < cardinal / 4; ++i) | ||
| { | ||
| test_count_true(v, eve::keep_between(i, i + cardinal / 4)); | ||
| test_count_true(v, eve::ignore_extrema(i, i + cardinal / 4)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where are the tests ignore everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just above, L98 and L87.
| } | ||
|
|
||
| template<typename T> | ||
| void test_count_true(T v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you say you now support not relative masks - I don't see tests.
No description provided.