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

Refactored local::promise to be standard conformant #1210

Merged
merged 1 commit into from Aug 4, 2014

Conversation

Projects
None yet
3 participants
@K-ballo
Member

K-ballo commented Aug 4, 2014

No description provided.

public:
promise_base()
: shared_state_(new shared_state_type())

This comment has been minimized.

@sithhell

sithhell Aug 4, 2014

Member

Do we really need an allocation here? Wouldn't it be more efficient to set to NULL here?

@sithhell

sithhell Aug 4, 2014

Member

Do we really need an allocation here? Wouldn't it be more efficient to set to NULL here?

This comment has been minimized.

@K-ballo

K-ballo Aug 4, 2014

Member

It would certainly be more efficient, but the specified effects for promise default constructor are to constructs a promise object and a shared state. promise is a weird beast like that. Do you suggest delaying the construction of the shared state until it's actually needed?

@K-ballo

K-ballo Aug 4, 2014

Member

It would certainly be more efficient, but the specified effects for promise default constructor are to constructs a promise object and a shared state. promise is a weird beast like that. Do you suggest delaying the construction of the shared state until it's actually needed?

This comment has been minimized.

@sithhell

sithhell Aug 4, 2014

Member

I think that should be done, yes.
I think the question additionally is: Is default constructed promise valid? It currently is.

@sithhell

sithhell Aug 4, 2014

Member

I think that should be done, yes.
I think the question additionally is: Is default constructed promise valid? It currently is.

This comment has been minimized.

@K-ballo

K-ballo Aug 4, 2014

Member

Ok, that could be arranged...
A default constructed promise is not only valid, but is the only way to obtain a valid promise.

@K-ballo

K-ballo Aug 4, 2014

Member

Ok, that could be arranged...
A default constructed promise is not only valid, but is the only way to obtain a valid promise.

This comment has been minimized.

@hkaiser

hkaiser Aug 4, 2014

Member

Does it make sense to delay the allocation? I can't think of a use case where the allocation wouldn't be needed at some point after all.

@hkaiser

hkaiser Aug 4, 2014

Member

Does it make sense to delay the allocation? I can't think of a use case where the allocation wouldn't be needed at some point after all.

This comment has been minimized.

@K-ballo

K-ballo Aug 4, 2014

Member

It'd be doable, since we don't have allocator support. Whether it be worth it, I'm not sure. I think it would only help for cases where a promise is destructed without ever being used.

@K-ballo

K-ballo Aug 4, 2014

Member

It'd be doable, since we don't have allocator support. Whether it be worth it, I'm not sure. I think it would only help for cases where a promise is destructed without ever being used.

This comment has been minimized.

@sithhell

sithhell Aug 4, 2014

Member

I was confused about if a default constructed promise is valid or not ... so let's keep it as is.

@sithhell

sithhell Aug 4, 2014

Member

I was confused about if a default constructed promise is valid or not ... so let's keep it as is.

private:
boost::intrusive_ptr<shared_state_type> shared_state_;
bool future_retrieved_;
bool has_result_;

This comment has been minimized.

@sithhell

sithhell Aug 4, 2014

Member

I guess having simple bools here is safe because promises aren't shared?

@sithhell

sithhell Aug 4, 2014

Member

I guess having simple bools here is safe because promises aren't shared?

This comment has been minimized.

@K-ballo

K-ballo Aug 4, 2014

Member

Nod, it's unique, there can't be copies.

@K-ballo

K-ballo Aug 4, 2014

Member

Nod, it's unique, there can't be copies.

@sithhell

This comment has been minimized.

Show comment
Hide comment
@sithhell

sithhell Aug 4, 2014

Member

Looks mostly good to me. I would like to see the comment to be confirmed before we merge this.

Member

sithhell commented Aug 4, 2014

Looks mostly good to me. I would like to see the comment to be confirmed before we merge this.

@hkaiser hkaiser added this to the 0.9.9 milestone Aug 4, 2014

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 4, 2014

Member

Looks good to me.

Member

hkaiser commented Aug 4, 2014

Looks good to me.

sithhell added a commit that referenced this pull request Aug 4, 2014

Merge pull request #1210 from STEllAR-GROUP/promise
Refactored local::promise to be standard conformant

@sithhell sithhell merged commit a8c8df2 into master Aug 4, 2014

1 check failed

default Merged build finished.
Details
@sithhell

This comment has been minimized.

Show comment
Hide comment
@sithhell

sithhell Aug 4, 2014

Member

Merged.

Member

sithhell commented Aug 4, 2014

Merged.

@K-ballo K-ballo deleted the promise branch Aug 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment