-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
Implemented parallel::stable_partition #2345
Conversation
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.
This is the first time I've used the 'review' facility, I'm just testing.
} | ||
|
||
/// Permutes the elements in the range [first, last) such that there exists | ||
/// and iterator i such that for every iterator j in the range [first, i) |
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.
'an' not 'and'
template <typename ExPolicy, typename IteratorTag> | ||
void test_stable_partition_async(ExPolicy p, IteratorTag) | ||
{ | ||
// typedef std::vector<int>::iterator base_iterator; |
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.
a lot of commented out code
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.
Uhh, and I was already wondering why my tests are all passing... ;-)
@biddisco All review comments have been addressed. |
- flyby addressing review comments - flyby generalize local exception handling for algorithms
9aba26c
to
a874c11
Compare
- flyby: add options for stricter conformance for MSVC
@@ -31,12 +31,14 @@ namespace hpx { namespace parallel { namespace util { namespace detail | |||
return T(); | |||
} | |||
|
|||
static type get(T && t) | |||
template <typename T_> | |||
static type get(T_ && t) |
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.
What's the rationale for this change?
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 need to revert this change and see what breaks to answer this question ;)
It looks at least strange since the other |
} | ||
|
||
HPX_TEST(caught_exception); | ||
HPX_TEST(returned_from_algorithm); |
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 suspect its remarkably unlikely (if the calling thread suspends), but the algorithm could throw before returned_from_algorithm = true;
is executed.
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.
The point of this this test is to make sure this does not happen. Exceptions have to be delivered through the returned future object.
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.
My point was that the algorithm is invoked and throws, but if the thread invoking the algorithm suspends immediately after calling it, the algorithm might throw before the returned_from_algorithm
flag is set. This would make the test fail (once every few million runs) even though the actaul test itself was still 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.
The idea is that even if the algorithm itself throws, the exception will never be visible on the calling thread until it calls future::get
. So even if the calling thread is suspended right before the flag is set, the test has to succeed as any exception thrown by the algorithm will be rethrown in the context of the calling thread later only.
This is related to #1141