This is an archive of the discontinued LLVM Phabricator instance.

Hot fix for equal algo
ClosedPublic

Authored by MikeDvorskiy on Mar 22 2019, 10:09 AM.

Diff Detail

Repository
rPSTL pstl

Event Timeline

MikeDvorskiy created this revision.Mar 22 2019, 10:09 AM
ldionne requested changes to this revision.Mar 22 2019, 10:44 AM
ldionne added inline comments.
include/pstl/internal/glue_algorithm_impl.h
739

Please restore the space here.

739

Why are you removing the TODO?

750

What problem is this fixing? IIUC, before this patch, we would end up calling equal(_ExecutionPolicy&&, _ForwardIterator1, _ForwardIterator1, _ForwardIterator2) here and that would be a compiler error because __pstl::internal::pstl_equal() is not an iterator. Is that correct?

If so, this change should be accompanied by a test that goes through this code path.

This revision now requires changes to proceed.Mar 22 2019, 10:44 AM
MikeDvorskiy marked an inline comment as done.Mar 25 2019, 2:06 AM
MikeDvorskiy added inline comments.
include/pstl/internal/glue_algorithm_impl.h
750

No...
The fact is there is several version of std::equal..
In particular,
equal(_ExecutionPolicy&&, _ForwardIterator1, _ForwardIterator1, _ForwardIterator2...
and
equal(_ExecutionPolicy&&, _ForwardIterator1, _ForwardIterator1, _ForwardIterator2, _ForwardIterator2...
where each sequence has its own length.
So, the error was the code calls the first version with "first1, last1, __first2"

MikeDvorskiy marked an inline comment as done.Mar 25 2019, 3:27 AM
MikeDvorskiy added inline comments.
include/pstl/internal/glue_algorithm_impl.h
739

"Distance check" is a kind of optimization for random access iterators..
But for forward iterators it is a kind of overhead.. and we should keep TODO complete the fixing.

Furthermore, I've just noticed, in case of size equal, we call a serial version std::equal(first1, last1, first2, p);

Ok, I'll complete the fixing and update the patch.
(and extend the test coverage as well)

ldionne accepted this revision.Mar 25 2019, 5:31 AM
This revision is now accepted and ready to land.Mar 25 2019, 5:31 AM
ldionne added inline comments.Mar 25 2019, 5:37 AM
include/pstl/internal/glue_algorithm_impl.h
739

Please fix this in a separate patch (I've already applied this one).

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 5:39 AM