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

ref: Avoid double-indirection through Arc<Box<T>> #380

Merged
merged 1 commit into from
Nov 3, 2021
Merged

Conversation

Swatinem
Copy link
Member

This also removes the need to manually box the argument to add_event_processor.

This also removes the need to manually box the argument to `add_event_processor`.
@Swatinem Swatinem requested a review from a team October 27, 2021 10:19
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

Does this still allow you to pass in a Box of a closure if you'd desire so?

@loewenheim
Copy link
Contributor

@flub A quick test on the playground seems to indicate so.

@flub
Copy link
Contributor

flub commented Oct 27, 2021

@flub A quick test on the playground seems to indicate so.

You forgot the Send + Sync + 'static bounds (especially that last one was what i was concerned about. but yess still works: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=511ce396202ba76002401e6ba278ad08

('d have approved right away, but github has taken that ability away from me)

@Swatinem
Copy link
Member Author

Technically, this is a breaking change, yes! But due to our all-at-once versioning policy, dependency updates (for example tracing-subscriber just now) means we publish semver-breaks all damn time. :-(

But also yes, if you have the proper types, then providing Box<dyn Fn> continues to work. The compiler just needs to know the type, and for some closures, it is not smart enough to infer.

@Swatinem Swatinem merged commit 82e212f into master Nov 3, 2021
@Swatinem Swatinem deleted the ref/box-ind branch November 3, 2021 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants