[oneDPL] + oneapi::dpl::__internal::__compare #1621
Conversation
MikeDvorskiy
force-pushed
the
dev/mdvorski/sort_lambda_fix
branch
from
2b501c9 to
e1bb26f
Compare
include/oneapi/dpl/pstl/utils.h
Outdated
| operator()(_Xp&& __x, _Yp&& __y) | ||
| { | ||
| return __comp(__proj(std::forward<_Xp>(__x)), __proj(std::forward<_Yp>(__y))); | ||
| } |
There was a problem hiding this comment.
Why not keep const& parameters as in the original lambda? I believe neither comparators nor projections are supposed to modify their arguments.
There was a problem hiding this comment.
good question. I intended to keep const&, but made approach at least consistency with the other similar predicates (__pstl_equal, __pstl_less, etc).
The next question why the other similar predicates accept T&&, not const T&... Probably, we had a compile issue with some tricky types (a kind proxy types etc). I will try to figure out it.
There was a problem hiding this comment.
Actually, I've remembered a case when we had a compilation issue when we had changed T&& by const T&. It was the binary_search.pass test. That code tries to call a lambda with T& argument, which is const T& in its turn.
So, keeping const T& for argument of __compare doesn't matche to operator __proj, which can be implemented by user asoperator()(T&) {}.
So, keeping T& we make oneDPL more robust relative to a user projection type definition.
There was a problem hiding this comment.
If I follow correctly, you are saying that using forwarding references here allows us to accommodate the case where a user provides a projection operator which takes as input a non-const lvalue ref (T&).
The question then follows: is this a good thing?
Our previous code did not enable this, it would be a compile time error. If a user's projection operator actually modifies the input reference, I believe that would result in UB in our sort routine (modified inputs every time the projection is applied). Conceptually projection operators shouldn't modify the input. So, what we are enabling is "convenience" for the user of not adding a const to their input params, and possibly letting a compile time error become a runtime error if their projection actually does modify the input.
There was a problem hiding this comment.
C++17 states that predicates (unary and binary) as well as comparators "shall/will not apply any non-constant function through the dereferenced iterator". For parallel algorithms, it additionally states that unary and binary operations also "shall not directly or indirectly modify objects via their arguments".
In C++20, projections, comparators, predicates etc. for range-based algorithms are required to satisfy the regular_invocable concept. That concept states that the function invocation "shall not modify the
function object or the arguments."
That's what I meant above saying that "neither comparators nor projections are supposed to modify their arguments." Even if user-provided function objects can technically have non-constant call operators/parameters, its only benefit seems to allow users "forget" const qualifiers - at the risk of having runtime errors, like Dan said above.
Unfortunately, both std libraries and Thrust are tolerant to function objects omitting const, and this is the main reason why we may need to do the same.
There was a problem hiding this comment.
(1) done
(2) It seems we tend to relax the requirement - to use T&& instead for const T& for input parameters - so, it doesn't break working user code.
There was a problem hiding this comment.
I think you misunderstood what I'm saying.
I agree this PR does not break working user code. The purpose of my comment is to state that it is "easy" to go in this direction. However, if we later were to change our minds and want to go back to const T&, from T&&, that could break working user code, so we would not make that change.
I'm just mentioning it to make it clear that we should be sure about our decision because it would be difficult to revert.
There was a problem hiding this comment.
I see your point, Dan....
Alexey (@akukanov), It seems we need a decision maker here.
There was a problem hiding this comment.
One more argument in favor of T&& - std::invoke accepts arguments as Args&&... args
There was a problem hiding this comment.
I am OK with the current patch. As for the plan - I do not object to revising the content of pstl/utils.h but I do not think it should be done for the same milestone.
MikeDvorskiy
force-pushed
the
dev/mdvorski/sort_lambda_fix
branch
from
c61dc9b to
ecd747c
Compare
…, + mutabale for comparator and projection fields.
…mpare - applying projection
|
I've looked through this PR as well as the parameter discussion and the current version of the patch looks good to me (assuming the minor outstanding comment is addressed). There is a CI failure but it looks to be from an outdated mac configuration with the no longer supported |
There was a problem hiding this comment.
LGTM,
I still prefer the version that adds the change suggested in this comment from @akukanov (I suggest it as a commit here for convenience), but I don't see it as a blocker, so I wont withhold my approval.
Co-authored-by: Dan Hoeflinger <[email protected]>
|
@danhoeflinger , could you please re-approve (and merge) after your suggestion was committed? |
| @@ -58,6 +58,9 @@ class __equal_value; | |||
| template <typename _Tp> | |||
| class __not_equal_value; | |||
|
|
|||
| template <typename _Comp, typename _Proj> | |||
| class __compare; | |||
There was a problem hiding this comment.
class -> struct to align with the definition.
This PR introduces oneapi::dpl::__internal::__compare which replace a lambda in hetero __parallel_stable_sort pattern