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

Remove unnecessary mutability in WatcherInterop #9688

Closed
wants to merge 1 commit into from

Conversation

flaper87
Copy link
Contributor

@flaper87 flaper87 commented Oct 2, 2013

Mutable instances are not necessary for WatcherInterop, the patch changes that.

@nikomatsakis r?

@alexcrichton
Copy link
Member

Are we sure that we want this? &mut provides stronger guarantees about how something is being accessed than & does, and it also expresses more of an interest about what the method is probably doing. I would expect a function like write to take &mut self because it's most likely mutating internal state. Especially for a function returning &mut I would expect the receiver to be &mut self as well.

I would consider it more of an implementation bug that all these structures are implicitly copyable and hence the &mut guarantees don't' mean a whole lot. By dropping &mut in as many places as possible, we're limiting ourselves to possible future implementations.

@flaper87
Copy link
Contributor Author

flaper87 commented Oct 3, 2013

I'm not that convinced about some of this methods needing to be &mut however, I agree that it may make sense to keep some of this as they are. However, I'd also like us to consider that must of these implementations required &mut just because WatcherInterop API required it - although it actually didn't need it to be mutable.

@alexcrichton
Copy link
Member

The three methods that are currently &mut are:

  • fn install_watcher_data(&mut self) - this makes sense to me to take &mut because you're modifying the receiver, you're installing data.
  • fn get_watcher_data<'r>(&'r mut self) -> &'r mut WatcherData - this needs the &mut because you're returning an interior &mut pointer
  • fn drop_watcher_data(&mut self) - similarly to installation, you're modifying the structure by dropping data, which makes sense to me to get &mut

This is a trait to implement, so the trait shouldn't expose implementation details. It may be the case that the implementation uses all unsafe pointer with a C api that completely disregards mutability, but the traits themselves should reflect the intention of the methods rather than the implementation details themselves.

@brson
Copy link
Contributor

brson commented Oct 4, 2013

In general, methods on uv wrappers take &mut even though the type system would allow them not to because natively they are performing some sort of mutation. The FFI isn't smart enough to know that these methods actually are mutating, so it's up to the discretion of the bindings to make sure that mutability is propagated correctly. So this is as intended.

I imagine that there are actually more methods taking &selfthat should rather be taking &mut self.

@brson brson closed this Oct 4, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 23, 2022
Move MSRV tests into the lint specific test files

There are currently two ways MSRV tests are done in the ui test suite, adding a case to the `#![clippy::msrv = "1.0"]` `tests/ui/min_rust_version_attr.rs` or adding the two `msrv_1_xx` functions to the test file of the lint in question

This updates the clippy book to suggest the `msrv_1_xx` style, and replaces the tests in `tests/ui/min_rust_version_attr.rs` with ones of that style

Almost the entire diff is just moving stuff around as a result, I made sure to check the line numbers the lints are emitted at correspond with the right `msrv` case, so feel free to only skim that part

changelog: none
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