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
Fix guided exec #3537
Fix guided exec #3537
Conversation
Fix execution:::post specialization to correctly overload the hint passing version.
@@ -126,7 +126,8 @@ namespace hpx { namespace threads | |||
HPX_FORCEINLINE | |||
typename std::enable_if< | |||
hpx::traits::is_threads_executor<Executor>::value && | |||
std::is_same<Hint, hpx::threads::thread_schedule_hint>::value | |||
std::is_same<typename hpx::util::decay<Hint>::type, | |||
hpx::threads::thread_schedule_hint>::value | |||
>::type | |||
post(Executor && exec, F && f, Ts &&... ts, Hint && hint) |
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.
How can there be an argument after a variadic one? I always assumed that the variadic list of arguments should be the last one.
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.
Sorry. Turns out I don't know what I'm doing after all.
Good job those variadic args are not actually used here. I've switched the order round now.
(gcc didn't mind about the variadic in the middle it seems)
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.
@biddisco may I suggest that you create a (completely separate) customization point for your executor that is enable_if'ed based on the guided_executor type? I think adding the additional condition on the scheduling hint may cause ambiguities.
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.
That's a good idea. Didn't think of that. I'll have a look later.
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 new customization point is probably too large a change to put into this release, but I will prepare one right away and have it ready for a patch release as soon as we need to address any problems that might crop up.
<< "async_execute : Hint : " | ||
<< util::debug::print_type<H>() << "\n"; | ||
/* << "async_execute : Hint : " | ||
<< util::debug::print_type<H>() << "\n"*/; |
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.
Please remove code that is not 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.
done
These patches appear to fix a couple of problems that sneaked through the guided executor merge.
Committing this to see if all tests pass with the slight tweak to the executor interface.