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

Allowing component clients as action return types #2184

Merged
merged 1 commit into from May 29, 2016
Merged

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented May 26, 2016

This fixes #2150

HPX_ASSERT(0 != dynamic_cast<
typed_continuation<typename util::decay<Arg0>::type> *
>(this));

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we decay in the static_cast as well? It's about the correct dynamic type of the continuation, not how the argument is passed to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately that will not work. If Arg0 is a const&, a static_cast to the decayed type will fail compiling as the trigger_value will expect a pure rvalue.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand the issue here completely. The dynamic cast fails if the type is not decayed, that means that the dynamic type is with the decayed type, right?
On the other hand, the compilation fails when using the static_cast with the decayed type?
This sounds like there is something very fishy going on there.

Copy link
Member Author

@hkaiser hkaiser May 27, 2016

Choose a reason for hiding this comment

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

It's difficult to explain. Let's assume Arg0 is deduced as id_type const&. In this case, the dynamic_cast<Arg0>(this) would fail as this is not of type typed_continuation<id_type const&> but of type typed_continuation<id_type>. Hence the decayed argument.

At the same time, if we statically casted this to typed_continuation<id_type>, then its member function trigger_value effectively would have the signature trigger_value(id_type&&) causing compilation problems (remember, trigger_value is a virtual function and we have no way to use a forwarding reference here - which would solve the issue).

One solution would be to have another virtual function trigger_value(Result const&) which would be used by the compiler in this case. Adding this however causes issues whenever Arg0 represents a move-only type (we can't 'disable' the virtual function from the type for this case).

This code is fishy as it does a static_cast<typed_continuation<id_type const&>>(this) where this is actually a typed_continuation<id_type>. This however should not be a problem as for both types the effectively generated class is 100% identical in layout and member function signatures.

Please let me know if you have any other suggestion on how to resolve the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Yuck...

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't it be typed_continuation<id_type> in the first place?

That's what I said. this points to an object of that type.

Do we really need forwarding references here? Aren't rvalue references enough? set_lco_value accepts rvalue references iirc.

That does not work if the function is called with a const&, didn't I mention that?

The easiest would be if you changed the static_cast to use the decayed type yourself. That would demonstrate the problem right away.

Copy link
Member

Choose a reason for hiding this comment

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

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'll play with it a little bit... the sanitizer should still complain about that code.

Right, it most likely will complain. But you could tell it to ignore this instance.

Copy link
Member

Choose a reason for hiding this comment

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

If you decay copy, everything works, here is the patch that works for me:
https://gist.github.com/sithhell/7c4f73280b236662e448b571a9218030

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, thanks.

@hkaiser hkaiser mentioned this pull request May 26, 2016
@hkaiser
Copy link
Member Author

hkaiser commented May 26, 2016

This also fixes #2182

@hkaiser hkaiser changed the title Allowing to component clients as action return types Allowing component clients as action return types May 27, 2016
@hkaiser hkaiser merged commit 2c28eb5 into master May 29, 2016
@hkaiser hkaiser deleted the fixing_2150 branch May 29, 2016 11:49
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.

Unable to use component clients as action return types
3 participants