-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
Implement parallel::unique_copy. #2754
Conversation
@taeguk: so is this ready to be reviewed now? |
@hkaiser I'm ready to be reviewed. |
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.
Otherwise, LGTM. Thanks!
hpx/parallel/algorithms/unique.hpp
Outdated
|
||
auto base_val = *first; | ||
|
||
*dest++ = *first; |
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.
You can't dereference input iterators more than once for the same 'position'.
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.
As I know, until input iterator is incremented, several dereferences are permitted.
GCC implementation : https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/stl_algo.h#L1093-L1097
MSVC14 (VS2015) implementation :
...
for (*_Dest++ = _Val; ++_First != _Last; )
if (!_Pred(_Val, *_First))
{ // copy unmatched
_Val = *_First;
*_Dest++ = _Val;
}
...
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.
Ok, I read the standard again - you're right. I'm not sure why I thought this to be undefined, I think I mixed it up with requirements for output iterators.
hpx/parallel/algorithms/unique.hpp
Outdated
hpx::util::invoke(proj, *first))) | ||
{ | ||
base_val = *first; | ||
*dest++ = *first; |
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.
Same here, calling *first
more than once without interleaving ++first
is not allowed for input iterators. Not sure if this is worth changing as we know this will not be called with input iterators in the first place...
Check Box
policy.executor()
instead ofhpx::launch::sync
inscan_partitioner
. (https://gist.github.com/taeguk/36f6fadce38c85096018f4df3a91d552)=> The parallel version is faster than the sequential version, but not satisfying.