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.

Sign up for GitHub

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

Jump to bottom

Remove channel in favour of accepting Fn in Watcher constructors #214

Merged
merged 7 commits into from
Oct 3, 2019
Merged

Remove channel in favour of accepting Fn in Watcher constructors #214

merged 7 commits into from
Oct 3, 2019

Conversation

mitchmindtree
Copy link
Contributor

@mitchmindtree mitchmindtree commented Sep 16, 2019 •
edited
Loading

This implements the changes described in #213.

This PR is a work-in-progress.

TODO:

  • Make API change.
  • Update inotify watcher.
  • Update poll watcher.
  • Update fsevents watcher.
  • Update windows watcher.

Closes #213.
Closes #160.

@mitchmindtree
Remove channel in favour of accepting Fn in Watcher constructors
39d7eb9 
This implements the changes described in notify-rs#213.

This PR is a **work-in-progress**.

TODO:

- [x] Make API change.
- [x] Update inotify watcher.
- [x] Update poll watcher.
- [ ] Update fsevents watcher.
- [ ] Update windows watcher.
@mitchmindtree
Copy link
Contributor Author

Oh also, I don't have access to macOS or Windows at the moment - if anyone has access to either of those platforms and would like to include them in this PR feel free! Otherwise it might be a while until I can address this.

@mitchmindtree
Copy link
Contributor Author

mitchmindtree commented Sep 23, 2019 •
edited
Loading

OK, I've managed to update the windows and fsevents Watcher implementations by adding windows and darwin targets via rustup and cargo checking those targets locally - I think this should be ready to go! Edit: If someone else could give this a review that would be greatly appreciated.

@mitchmindtree
Box the inotify backend event_fn to manage downstream compile times
ec9c57c 
By boxing the EventFn field within the inotify EventLoop we can stop
the need to regenerate the item's layout each time a change occurs
within the user's EventFn implementation.
@mitchmindtree
Copy link
Contributor Author

@matklad ah I just noticed you requested a change similar to this in #160 for rust-analyser. Would you be interested in reviewing this?

matklad
matklad reviewed Sep 23, 2019
View reviewed changes
Copy link

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Did a cursory review, but I don't really understand notify's internals, so some high-level bits might be missing

examples/monitor_raw.rs Outdated Show resolved Hide resolved
src/fsevent.rs Show resolved Hide resolved
src/inotify.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated

impl<F> EventFn for F
where
F: 'static + Fn(Result<Event>) + Send + Sync {}
Copy link

Choose a reason for hiding this comment

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

I'd use something like type OnEvent = dyn Fn(Result<Event>) + Send + Sync + 'static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip, but does this matter much? Are there bigger motivations for using the type alias, or is it mostly just to avoid having to prefix dyn everywhere? I'm currently leaning towards leaving this as it is, but want to check I'm not missing some subtly incurred cost by leaving it this way.

Copy link

Choose a reason for hiding this comment

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

It’s not to avoid dyn, it’s to avoid an extra trait and just lean on existing Fn

src/lib.rs Show resolved Hide resolved
src/poll.rs Outdated Show resolved Hide resolved
@mitchmindtree
Avoid large monomorphisation in PollWatcher via earlier dyn dispatch
fa58dfc
@mitchmindtree
Fix synchronisation in examples and address over monomorphising
5c1467a 
This adds the indirection of dynamic dispatch as early as possible
within the `Watcher` implementations to avoid generating large amounts
of code for calls to `Watcher` methods with different argument types.

Also addresses some synchronisation issues raised by @matklad in some
examples and tests and changes them to match their original behaviour.
@mitchmindtree
Copy link
Contributor Author

mitchmindtree commented Sep 24, 2019 •
edited
Loading

Thanks again @matklad, I think I've addressed everything you have raised.

If there is nothing else, I might merge this within the next couple of days.

A nice follow-up to this PR might be to see if crossbeam is necessary at all any longer. It maybe be possible to remove the remaining use cases in favour of std in order to shave off a dependency? Edit: particularly as it is no longer publicly exposed.

@passcod passcod mentioned this pull request Sep 24, 2019
Closed
@mitchmindtree mitchmindtree merged commit 97a870f into notify-rs:main Oct 3, 2019
@mitchmindtree mitchmindtree mentioned this pull request Oct 13, 2019
Closed
@JohnTitor JohnTitor mentioned this pull request Jan 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Have Watcher::new accept a function rather than a channel Use crossbeam-channel instead of stdlib
2 participants
@mitchmindtree @matklad