-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
Adapted parallel::{search | search_n} for Ranges TS (see #1668) #3210
Conversation
…Updated compare projection to take two different types of projections
hpx/parallel/algorithms/search.hpp
Outdated
traits::is_projected<Proj1, FwdIter>::value && | ||
hpx::traits::is_iterator<FwdIter2>::value && | ||
traits::is_projected<Proj2, FwdIter2>::value && | ||
std::is_convertible< |
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.
Hmm.. I don't think that FwdIter2
's thing and FwdIter
's thing must be convertible each other because BinaryPredicate
doesn't require 'strict weak ordering' unlike Compare
concepts.
But I'm not sure. I can't find the clarified thing in c++ standard draft document...
But, the standard just says
In other words, if an algorithm takes BinaryPredicate binary_pred as its argument and first1 and first2 as its iterator arguments, it should work correctly in the construct binary_pred(*first1, *first2) contextually converted to bool (Clause 4).
Page 896 of http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf
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.
So Clause 4 discusses implicit conversions which is why I think that addition is necessary.
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.
@cogle Clause 4 discusses about conversion to Boolean value. This is not related to our focus. (The sentence what I refer is not 'Clause 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.
@cogle The sentence what I refer just says about binary_pred(*first1, *first2)
. There is no content about binary_pred(*first2, *first1)
.
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 see. I asked stack overflow about this to see if somebody on there had any insight I to why that site would explicitly state that the two must be implicitly convertible, and will update with and answer if I get one.
hpx/parallel/algorithms/search.hpp
Outdated
@@ -98,7 +118,8 @@ namespace hpx {namespace parallel { inline namespace v1 | |||
local_count != diff && len != count; | |||
++local_count, ++len, ++mid) | |||
{ | |||
if(*mid != *++needle) | |||
if(hpx::util::invoke(proj1, *mid) != |
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.
Using op
rather than !=
seems correct.
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.
Great catch!
hpx/parallel/algorithms/search.hpp
Outdated
traits::is_projected<Proj1, FwdIter>::value && | ||
hpx::traits::is_iterator<FwdIter2>::value && | ||
traits::is_projected<Proj2, FwdIter2>::value && | ||
std::is_convertible< |
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 thing.
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.
Great catch!
@taeguk let me know of any other issues such as poor formatting or other misc things. |
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.
Thanks, LGTM 👍
This is further continuation of my work with regards to issue #1668. For this particular pull request please take your time with the review process as this is my largest contribution to the library thus far, and I did make some changes to the existing library most notably, to
hpx/parallel/util/compare_projected.hpp
in order to extend it such that it can take two distinct projections. In addition I added an extra constraint to the search(_n) algorithm's template parameter list, enforcing that the two respective iterators are implicitly convertible which is a constraint stated here.Edit: I have since removed the implicit convertible check as it appears after consulting with stack overflow that this an error in the website's template.