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

Allow for default constructed parcel instances to be moved #1792

Merged
merged 2 commits into from Oct 14, 2015

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Oct 8, 2015

This fixes #1790: assertion 'action_.get()' failed: HPX(assertion_failure) when running Octotiger with pull request 1786

@sithhell
Copy link
Member

sithhell commented Oct 8, 2015

The asserts were put in for sanity checks. A parcel without an action doesn't make a lot of sense. There might be more behind #1790, removing those asserts might just cover them up

@hkaiser
Copy link
Member Author

hkaiser commented Oct 8, 2015

We move default constructed parcels, for those these asserts will fire.

@hkaiser
Copy link
Member Author

hkaiser commented Oct 8, 2015

It would make more sense to check for an action only if we access it in order to invoke it, instead.

@sithhell
Copy link
Member

sithhell commented Oct 9, 2015

Am 09.10.2015 1:41 vorm. schrieb "Hartmut Kaiser" <notifications@github.com

We move default constructed parcels, for those these asserts will fire.

Can you show me where this is happening? I find it highly suspicious that
those suddenly fire, I would like to know why this of suddenly happening
before removing it.

@sithhell
Copy link
Member

sithhell commented Oct 9, 2015

Am 09.10.2015 1:42 vorm. schrieb "Hartmut Kaiser" <notifications@github.com

It would make more sense to check for an action only if we access it in
order to invoke it, instead.

Why? This catches a potential error as early as possible, checking on each
access sounds excessive.

@hkaiser
Copy link
Member Author

hkaiser commented Oct 9, 2015

It would make more sense to check for an action only if we access it in order to invoke it, instead.

Why? This catches a potential error as early as possible, checking on each access sounds excessive.

Let's add a proper check whether the parcel is valid, instead. Just the absence of a valid action does not tell anything.

@hkaiser
Copy link
Member Author

hkaiser commented Oct 9, 2015

Can you show me where this is happening? I find it highly suspicious that
those suddenly fire, I would like to know why this of suddenly happening
before removing it.

We do that in several places, for instance here.

@sithhell
Copy link
Member

The callback takes the parcel by reference, no?
In any case, I am just very curious why this assert hasn't fired so far on master. I think #1790 has a deeper underlying problem which needs to be fixed instead. We can of course move the assert to check on each access of the action, but as said, this sounds excessive and unnecessary to me.

@hkaiser
Copy link
Member Author

hkaiser commented Oct 13, 2015

We can of course move the assert to check on each access of the action, but as said, this sounds excessive and unnecessary to me.

That's done already here.

@hkaiser
Copy link
Member Author

hkaiser commented Oct 13, 2015

The latest commit adds more comprehensive sanity checking to parcels not relying on the action only.

@sithhell
Copy link
Member

LGTM, thanks!

sithhell added a commit that referenced this pull request Oct 14, 2015
Allow for default constructed parcel instances to be moved
@sithhell sithhell merged commit 78096d6 into master Oct 14, 2015
@sithhell sithhell deleted the fixing_1790 branch October 14, 2015 21:52
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.

assertion 'action_.get()' failed: HPX(assertion_failure) when running Octotiger with pull request 1786
2 participants