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

mutex_atomic and CondVar #1516

Open
Yamakaky opened this issue Feb 6, 2017 · 7 comments
Open

mutex_atomic and CondVar #1516

Yamakaky opened this issue Feb 6, 2017 · 7 comments
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@Yamakaky
Copy link

Yamakaky commented Feb 6, 2017

The example at https://doc.rust-lang.org/std/sync/struct.Condvar.html issues a mutex_atomic warning, suggesting the replacement of Mutex::new(false) by an AtomicBool. My understanding is that we need a mutex to use CondVar.

@kaimast
Copy link

kaimast commented Feb 23, 2020

Still a problem in most recent nightly...

@flip1995 flip1995 added L-suggestion Lint: Improving, adding or fixing lint suggestions E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied and removed L-suggestion Lint: Improving, adding or fixing lint suggestions labels Feb 23, 2020
@gliderkite
Copy link

This is still reported as warning in Rust 1.45

asherkin added a commit to asherkin/discograph that referenced this issue Aug 3, 2020
@x4e
Copy link

x4e commented May 28, 2021

Related to #4295.
I think perhaps this warning is slightly too easy to trigger. It would be far too complicated to statically determine if an AtomicBoolean is actually more appropriate than a Mutex. Perhaps this lint should be changed to an allow level to represent its high false positive rate?

@taiki-e
Copy link
Member

taiki-e commented Jan 10, 2022

Filed #8260 to downgrade this lint to nursery group that allowed by default.

@llogiq
Copy link
Contributor

llogiq commented Jan 10, 2022

Is that the correct solution here?

In my experience I get a bunch of newbies who don't understand sync primitives triggering this lint and then there are a handful of people who actually know better. Making the lint allow by default will mean those in the know don't need to do anything while the others don't know anything to do.

I agree that the suggestion is suboptimal and should likely be removed in favor of better docs.

@taiki-e
Copy link
Member

taiki-e commented Jan 11, 2022

@llogiq
Personally, I agree with removing this lint, but it was proposed in the past and two maintainers effectively rejected it (#4295 (comment), #4295 (comment)).
However, no one has worked on fixing the bugs in this lint, and several people have written the wrong code due to this lint.
I don't have the energy to discuss this with the maintainers who are opposed to removing lint, so I suggested a downgrade as a compromise.

@llogiq
Copy link
Contributor

llogiq commented Jan 11, 2022

Ok, so I'm going to accept the downgrade as a temporary measure until the suggestion and docs have been sorted out.

bors added a commit that referenced this issue Jan 11, 2022
Downgrade mutex_atomic to nursery

See #1516 and #4295.

There are suggestions about removing this lint from the default warned lints in both issues.
Also, [`mutex_integer`](https://rust-lang.github.io/rust-clippy/master/index.html#mutex_integer) lint that has the same problems as this lint is in `nursery` group.

changelog: Moved [`mutex_atomic`] to `nursery`
ruuda added a commit to ruuda/musium that referenced this issue May 21, 2023
Clippy's advice is incorrect here, we need a Mutex for the Condvar to be
able to use Condvar::wait.

See also:

 * rust-lang/rust-clippy#1516
 * rust-lang/rust-clippy#4295 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

No branches or pull requests

7 participants