[oneDPL][ranges][tests] a fix for std::ranges::<algo> using with Windows C++ standard library. #1920
Conversation
MikeDvorskiy
force-pushed
the
dev/mdvorski/std_ranges_test_sort_fix_windows
branch
2 times, most recently
from
a4b1db9
to
6f8de9b
Compare
| @@ -21,18 +21,24 @@ main() | |||
| #if _ENABLE_STD_RANGES_TESTING | |||
| using namespace test_std_ranges; | |||
| namespace dpl_ranges = oneapi::dpl::ranges; | |||
| auto sort_checker = | |||
| #if _ONEDPL_STD_RANGES_ALGO_CPP_FUN | |||
There was a problem hiding this comment.
Could you introduce a similar macro in the tests in order not to rely on oneDPL internals?
There was a problem hiding this comment.
I still strong disagree to make the code duplication.
Rather lets consider a an option to make public that macro....
There was a problem hiding this comment.
I do not think that having a public macro for that is a good idea given that it is a workaround for a niche issue, not resulting in enablement of any oneDPL feature. Also, I understand that duplication increases maintenance burden,
but I would duplicate this small code section to decouple the tests and implementation. I guess we need a mediator here.
There was a problem hiding this comment.
Relocating my comment here, I missed this discussion in my initial review. I agree with Dmitriy here, this has been the convention as established by @akukanov.
We have tried to stay away from using internal helpers / utilities / implementation details in our tests.
If we want to use the same macro here, I think we probably want to re-define it here test/support/utils.h, or somewhere similar as _TEST_PREPARE_CALLABLE, or something like that, then use that throughout the tests.
MikeDvorskiy
changed the title
| @@ -333,4 +333,11 @@ | |||
| # define _ONEDPL_STD_RANGES_ALGO_CPP_FUN 0 | |||
| #endif | |||
|
|
|||
| #if _ONEDPL_STD_RANGES_ALGO_CPP_FUN | |||
| # define _ONEDPL_ALGO(std_algo_name) \ | |||
| [](auto&&... __args) { return std_algo_name(std::forward<decltype(__args)>(__args)...); } | |||
There was a problem hiding this comment.
Will not it be better to move that utility in a separate header, e.g. utils.h?
A "config" usually contains well some settings, e.g. to enable workarounds, rather than workarounds themselves. Also, having a macro (which are generelly avoided) looks unnecessary to resolve that particular issue.
There was a problem hiding this comment.
To tell the truth, practically, many macro wrappers were accumulated in onedpl_config.h. F.e. a lot of _ONEDPL_PRAGMA_SIMD wrappers. Proposed _ONEDPL_ALGO is a kind of macro wrapper as _ONEDPL_PRAGMA_SIMD.
File utils.h, traditionally, contains C/C++ functions/utilities.
There was a problem hiding this comment.
I implied creation of a function instead of a macro in utils.h, but perhaps it is a matter of taste, so I do not insist.
There was a problem hiding this comment.
No, it is not "it is a matter of taste"...
Of course I prefer a function too... But there is no way to write a function here.
There was a problem hiding this comment.
But it is possible to write a function in utils.h, if I understood you correctly.
There was a problem hiding this comment.
I think I agree that this is more of a "utility" than a "configuration". I understand the motivation to group preprocessor macros together, and utility functions together, but the concept seems more important to me.
I think I would be in favor of keeping _ONEDPL_STD_RANGES_ALGO_CPP_FUN definition here, and moving the rest to "utils.h", but I also wont insist.
| @@ -333,4 +333,11 @@ | |||
| # define _ONEDPL_STD_RANGES_ALGO_CPP_FUN 0 | |||
| #endif | |||
|
|
|||
| #if _ONEDPL_STD_RANGES_ALGO_CPP_FUN | |||
| # define _ONEDPL_ALGO(std_algo_name) \ | |||
There was a problem hiding this comment.
Nitpick: _ONEDPL_ALGO is not very descriptive.
Something like _ONEDPL_PREPARE_FUNCTION_OBJ, or _ONEDPL_PREPARE_CALLABLE or something would be more descriptive.
There was a problem hiding this comment.
I don't mind _ONEDPL_PREPARE_CALLABLE macro name
|
I think I agree with the clang-format suggestions, but not a blocker for me. |
… standard C++ library
…windows standard C++ library
…dows standard C++ library
MikeDvorskiy
force-pushed
the
dev/mdvorski/std_ranges_test_sort_fix_windows
branch
from
0188ba1
to
d6b8e9b
Compare
MikeDvorskiy
force-pushed
the
dev/mdvorski/std_ranges_test_sort_fix_windows
branch
from
d6b8e9b
to
322e0ea
Compare
[oneDPL][ranges][tests] a fix for std::ranges::sort using with Windows C++ standard library.
This PR is a sequel for #1885.