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

Fixing shared_executor_test #1992

Merged
merged 3 commits into from Feb 20, 2016

Conversation

Projects
None yet
3 participants
@hkaiser
Copy link
Member

commented Feb 18, 2016

  • flyby: adding missing typedef
Fixing shared_executor_test
- flyby: adding missing typedef
typename hpx::util::result_of<
typename hpx::util::decay<F>::type()
>::type
>::type

This comment has been minimized.

Copy link
@sithhell

sithhell Feb 18, 2016

Member

Shouldn't this be rather done in the trait? Is it an error in general to return a reference from the passed function?

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 18, 2016

Author Member

I tried solving it in the trait, but was not able to do it there. If you have an idea how this could be formulated, I'd appreciate any ideas.

This comment has been minimized.

Copy link
@sithhell

sithhell Feb 18, 2016

Member

I tried solving it in the trait, but was not able to do it there. If you
have an idea how this could be formulated, I'd appreciate any ideas.

I am still trying to wrap my around why this is needed in the first place.

This comment has been minimized.

Copy link
@K-ballo

K-ballo Feb 18, 2016

Member

Tampering with the result like this looks wrong to me. It will also make some valid corner cases ill-formed.

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 18, 2016

Author Member

I agree. Any suggestions on how to solve this? The patch here was just meant as a stop-gap measure to make tests pass. Any proper solution would be certainly preferred.

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 18, 2016

Author Member

I tried solving it in the trait, but was not able to do it there. If you
have an idea how this could be formulated, I'd appreciate any ideas.

I am still trying to wrap my around why this is needed in the first place.

shared_future<>::get() returns a const& to the result. This code extracts the result, passing it along, the shared_future itself however goes out of scope.

@K-ballo

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

Intuitively I would expect execute_helper::run to have a return type that matches that of the decayed function invocation. Perhaps:

            typename hpx::util::result_of<
                typename hpx::util::decay<F>::type()
            >::type

That might show (real) issues when shared_future is involved though.

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2016

That might show (real) issues when shared_future is involved though.

That is exactly the problem we're seeing. The current code uses auto-returntype deduction which amounts to the same things as you suggested, causing the code to return a dangling reference to the user of executor_traits<Executor>::execute()

@K-ballo

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

I don't think that's the case, if F() returns a reference type then shared_future<R&>::get() will return a reference that will only dangle if F() itself does.

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2016

I don't think that's the case, if F() returns a reference type then shared_future<R&>::get() will return a reference that will only dangle if F() itself does.

Sure. The problem lies in how the trait emulates the synchronous execute:

{
    return exec.async_execute(std::forward<F>(f)).get();
}

which will - as you said - dangle the reference.

Additionally, as you said in IRC, the shared_future<> will prevent us from using move-only types. So I'm not sure on how to create a generic emulation in the trait class supporting all of the use cases without the ugly hack I proposed in this PR.

@K-ballo

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

The ugly hack presented here already prevents the use of move-only types, there's no way around that with shared_future.

Without knowing much about executor_traits, if the point of call is an attempt to implement synchronous calls on top of the existing interface then the implementation doesn't quite work either, as it decays the callable. That might end up picking the wrong overload, or simply be ill-formed (assuming call does not document any non-obvious requirements).

Wrapping the callable with ref might help, but it doesn't solve lack of support for movable only return types.

Moving solution to shared_executor problem to traits
- Also: fixed return types for executor::execute() functions
- added feature test for experimental/optional
- added explicit implementation of execute for distribution_policy_executor

@hkaiser hkaiser force-pushed the shared_executor_test branch from 0bc921a to af36acf Feb 18, 2016

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2016

@sithhell, @K-ballo: I think your comments have been addressed.

@sithhell

This comment has been minimized.

Copy link
Member

commented Feb 20, 2016

hkaiser added a commit that referenced this pull request Feb 20, 2016

@hkaiser hkaiser merged commit b54cfd1 into master Feb 20, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@hkaiser hkaiser deleted the shared_executor_test branch Feb 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.