[clang-tidy] New check: Prefer `std::scoped_lock` over `std::lock_guard`

[SunBlack]

In addition to the Clang-Tidy and compiler warnings, we also use other static analysis tools. One of them points out that with C++17 there is actually no longer any reason to use std::lock_guard, since std::scoped_lock is supposed to be identical in terms of performance, but also uses additional deadlock protection mechanisms if several mutexe are to be locked at the same time. You should therefore always use std::scoped_lock so that it is more uniform in the code, even if you may not have any advantages in a specific case.

Example:

std::lock_guard lock(myMutex);
// a bit code
std::lock_guard lock2(myMutex2);
std::lock_guard lock3(myMutex3);

Automatic fix could lock like this:

std::scoped_lock lock(myMutex);
// a bit code
std::scoped_lock lock2(myMutex2, myMutex3);

Notes:

• I'm not sure if an auto fix can merge multiple std::scoped_lock in a row, as I don't know if it's okay to automatically use the variable name of the first std::scoped_lock, or if it would be better to make it 2 checks instead: readability-prefer-scoped_lock & bugprone-uncombined-scoped_lock.
• There is also the std::unique_lock, where you can often use std::scoped_lock instead (if the lock does not have to be moved and does not get any extra parameters). This transition is not so trivial, but would possibly speak in favor of packing the transition and the merging of locks into 2 different checks.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: SunBlack (SunBlack)

In addition to the Clang-Tidy and compiler warnings, we also use other static analysis tools. One of them points out that with C++17 there is actually no longer any reason to use `std::lock_guard`, since `std::scoped_lock` is supposed to be identical in terms of performance, but also uses additional deadlock protection mechanisms if several mutexe are to be locked at the same time. You should therefore always use `std::scoped_lock` so that it is more uniform in the code, even if you may not have any advantages in a specific case.

Example:

std::lock_guard lock(myMutex);
// a bit code
std::lock_guard lock2(myMutex2);
std::lock_guard lock3(myMutex3);

Automatic fix could lock like this:

std::scoped_lock lock(myMutex);
// a bit code
std::scoped_lock lock2(myMutex2, myMutex3);

Notes:

• I'm not sure if an auto fix can merge multiple std::scoped_lock in a row, as I don't know if it's okay to automatically use the variable name of the first std::scoped_lock, or if it would be better to make it 2 checks instead: readability-prefer-scoped_lock & bugprone-uncombined-scoped_lock.
• There is also the std::unique_lock, where you can often use std::scoped_lock instead (if the lock does not have to be moved and does not get any extra parameters). This transition is not so trivial, but would possibly speak in favor of packing the transition and the merging of locks into 2 different checks.

[carlosgalvezp]

This sounds like it could fit in the modernize category? E.g. modernize-use-scoped-lock

[HerrCai0907]

I think maybe for the normal user (who don't want very strict rules), it is also good to diag they use continuous lock_guard instead of a combined scoped_lock.

[tamird]

https://en.cppreference.com/w/cpp/thread/scoped_lock:

If several mutexes are given, deadlock avoidance algorithm is used as if by std::lock.

https://en.cppreference.com/w/cpp/thread/lock:

The objects are locked by an unspecified series of calls to lock, try_lock, and unlock.

I don't think it's wise to encourage developers to rely on inconsistent lock ordering being permitted.

Perhaps this check should be off by default.

[SunBlack]

The objects are locked by an unspecified series of calls to lock, try_lock, and unlock.

I don't think it's wise to encourage developers to rely on inconsistent lock ordering being permitted.

Do you have an example of where this would be a issue? I think they mean that the first mutex is not locked first, then you wait until the second one can be locked, but that there is no fixed order and it is only guaranteed that it works. SonarLint says to it:

It can lock several mutexes at the same time, with a deadlock prevention mechanism