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

Move mutex_integer to restriction and improve mutex_{integer,atomic} docs #14110

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Move mutex_integer to restriction and improve mutex_{integer,atomic} docs #14110

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions clippy_lints/src/mutex_atomic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@ declare_clippy_lint! {
///
/// On the other hand, `Mutex`es are, in general, easier to
/// verify correctness. An atomic does not behave the same as
/// an equivalent mutex. See [this issue](https://github.com/rust-lang/rust-clippy/issues/4295)'s commentary for more details.
/// an equivalent mutex. See [this issue](https://github.com/rust-lang/rust-clippy/issues/4295)'s
/// commentary for more details.
///
/// ### Known problems
/// This lint cannot detect if the mutex is actually used
/// for waiting before a critical section.
/// * This lint cannot detect if the mutex is actually used
/// for waiting before a critical section.
/// * This lint has a false positive that warns without considering the case
/// where `Mutex` is used together with `Condvar`.
///
/// ### Example
/// ```no_run
Expand All @@ -48,14 +51,23 @@ declare_clippy_lint! {
/// Checks for usage of `Mutex<X>` where `X` is an integral
/// type.
///
/// ### Why is this bad?
/// ### Why restrict this?
/// Using a mutex just to make access to a plain integer
/// sequential is
/// shooting flies with cannons. `std::sync::atomic::AtomicUsize` is leaner and faster.
///
/// On the other hand, `Mutex`es are, in general, easier to
/// verify correctness. An atomic does not behave the same as
/// an equivalent mutex. See [this issue](https://github.com/rust-lang/rust-clippy/issues/4295)'s
/// commentary for more details.
///
/// ### Known problems
/// This lint cannot detect if the mutex is actually used
/// for waiting before a critical section.
/// * This lint cannot detect if the mutex is actually used
/// for waiting before a critical section.
/// * This lint has a false positive that warns without considering the case
/// where `Mutex` is used together with `Condvar`.
/// * This lint suggest using `AtomicU64` instead of `Mutex<u64>`, but
/// `AtomicU64` is not available on some 32-bit platforms.
///
/// ### Example
/// ```no_run
Expand All @@ -70,7 +82,7 @@ declare_clippy_lint! {
/// ```
#[clippy::version = "pre 1.29.0"]
pub MUTEX_INTEGER,
nursery,
restriction,
"using a mutex for an integer type"
}

Expand Down Expand Up @@ -108,7 +120,7 @@ fn get_atomic_name(ty: Ty<'_>) -> Option<&'static str> {
UintTy::U32 => Some("AtomicU32"),
UintTy::U64 => Some("AtomicU64"),
UintTy::Usize => Some("AtomicUsize"),
// There's no `AtomicU128`.
// `AtomicU128` is unstable and only available on a few platforms: https://github.com/rust-lang/rust/issues/99069
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this lint could be configurable:

  • You may want to specify a maximum size of integer that would trigger the lint (for example 32 bits) if you want your code to be portable across various architectures you use, with a reasonable default (32 or 64).
  • You may want to check if the projected target type exist on the current target at least, by looking up the corresponding diagnostic item. Note that AtomicI128 and AtomicU128 are also diagnostic items, but you may also want to check for the "integer_atomics" feature before you are going to suggest them.

UintTy::U128 => None,
}
},
Expand All @@ -119,7 +131,7 @@ fn get_atomic_name(ty: Ty<'_>) -> Option<&'static str> {
IntTy::I32 => Some("AtomicI32"),
IntTy::I64 => Some("AtomicI64"),
IntTy::Isize => Some("AtomicIsize"),
// There's no `AtomicI128`.
// `AtomicU128` is unstable and only available on a few platforms: https://github.com/rust-lang/rust/issues/99069
IntTy::I128 => None,
}
},
Expand Down