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

Simplify basic action implementations #3679

Merged
merged 6 commits into from Feb 12, 2019
Merged

Conversation

K-ballo
Copy link
Member

@K-ballo K-ballo commented Feb 9, 2019

No description provided.

typedef actions::action<
void (*)(std::exception_ptr const&), console_error_sink
> console_error_sink_action;
HPX_DEFINE_PLAIN_ACTION(console_error_sink, console_error_sink_action);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? and is it the correct way to declare such an action?

hkaiser
hkaiser previously approved these changes Feb 10, 2019
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks!

@sithhell
Copy link
Member

clang doesn't like all of your changes:

/hpx/source/hpx/runtime/actions/component_action.hpp:91:5: error: 'action' defined as a class template here but previously declared as a struct template [-Werror,-Wmismatched-tags]
    class action<R (Component::*)(Ps...), F, Derived>
    ^
/hpx/source/hpx/runtime/actions/basic_action.hpp:528:5: note: did you mean class here?
    struct action;
    ^~~~~~
    class

@hkaiser
Copy link
Member

hkaiser commented Feb 11, 2019

There are also additional test failures caused by the proposed changes: https://stellar-group.gitlab.io/-/hpx/-/jobs/159443858/artifacts/tests.unit.html#775cc3d3-3e99-462e-91fa-e374cdeb7c28

@sithhell sithhell merged commit fb273f2 into master Feb 12, 2019
@sithhell sithhell deleted the basic_action-simplify branch February 12, 2019 06:25
@hkaiser
Copy link
Member

hkaiser commented Feb 12, 2019

@sithhell, @K-ballo: were the test failures referenced above fixed now?

@K-ballo
Copy link
Member Author

K-ballo commented Feb 12, 2019

@K-ballo: were the test failures referenced above fixed now?

Yes, by that commit after the reference. A const-qualified component type was sometimes used to do the pinning whereas a mutable component is required, and since pinning is SFINAE-based it would simply ignore the request instead of diagnosing the issue.

@msimberg msimberg added this to the 1.3.0 milestone May 2, 2019
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.

None yet

4 participants