Skip to content
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

Merged
merged 3 commits into from
Oct 9, 2016
Merged

Implemented parallel::stable_partition #2345

merged 3 commits into from
Oct 9, 2016

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Sep 26, 2016

This is related to #1141

Copy link
Contributor

@biddisco biddisco left a 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)
Copy link
Contributor

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;
Copy link
Contributor

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

Copy link
Member Author

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... ;-)

@hkaiser
Copy link
Member Author

hkaiser commented Sep 27, 2016

@biddisco All review comments have been addressed.

- flyby addressing review comments
- flyby generalize local exception handling for algorithms
@hkaiser hkaiser force-pushed the partition branch 2 times, most recently from 9aba26c to a874c11 Compare September 28, 2016 21:29
- 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)
Copy link
Member

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?

Copy link
Member Author

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 ;)

@sithhell
Copy link
Member

sithhell commented Oct 4, 2016

I need to revert this change and see what breaks to answer this question ;)

It looks at least strange since the other get calls expect rvalues ;)

}

HPX_TEST(caught_exception);
HPX_TEST(returned_from_algorithm);
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Member Author

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.

@biddisco biddisco merged commit a27ad9e into master Oct 9, 2016
@biddisco biddisco deleted the partition branch October 9, 2016 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants