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 tag_invoke and friends #5332

Merged
merged 1 commit into from
May 18, 2021
Merged

Simplify tag_invoke and friends #5332

merged 1 commit into from
May 18, 2021

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented May 14, 2021

The goal is to streamline the implementation of tag_invoke and friends.

@msimberg, @K-ballo, if you could spare a minute, I'd appreciate a second pair of eyes as I'm not sure that the proposed changes will not subtly change overload resolution.

@hkaiser
Copy link
Member Author

hkaiser commented May 15, 2021

@msimberg not sure where the 443_executor test problem comes from, but otherwise things seem to be fine now.

@hkaiser hkaiser added this to the 1.7.0 milestone May 15, 2021
@hkaiser
Copy link
Member Author

hkaiser commented May 16, 2021

retest lsu

@msimberg
Copy link
Contributor

@msimberg not sure where the 443_executor test problem comes from, but otherwise things seem to be fine now.

Not sure since it's probably from the previous CI run, but most likely it's the same failure that was mentioned earlier and is fixed by #5274?

@msimberg
Copy link
Contributor

@hkaiser would you mind summarizing the changes here. It looks like the tag_X_invoke_ns namespace became regular (non-inline) namespaces, the helper base classes moved into sub-namespaces, while being typedefed in separate inline namespaces. Is this to prevent certain accidental/unwanted overloads matching or similar? Is there some other change that I've missed?

@hkaiser
Copy link
Member Author

hkaiser commented May 17, 2021

@hkaiser would you mind summarizing the changes here. It looks like the tag_X_invoke_ns namespace became regular (non-inline) namespaces, the helper base classes moved into sub-namespaces, while being typedefed in separate inline namespaces. Is this to prevent certain accidental/unwanted overloads matching or similar? Is there some other change that I've missed?

Yes, this mostly sums it up. The main change is that the helper base classes now directly dispatch to the tag_invoke implementations instead of using hpx::functional::tag_invoke as before. This saves one function call for each tag_invoke invocation. The namespace changes were needed to avoid the roundtrip through the hpx::functional::tag_invoke facility.

@msimberg
Copy link
Contributor

Thanks @hkaiser! I think we can ignore the ICC failures. As far as I can tell they only happen because this is still using the configuration from before #5199 was merged.

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.

2 participants