About cpp:S1199 and scoped lock

[FlorentP42]

Context

• ALM: GitHub
• Language: C++
• URL: SonarCloud

Observed behavior

Rule cpp:S1199 is reporting the usage of any free scope as a bad practice, however it appears to me as a very useful an common usage to use them for scoped mutex locks (and other development situations making good use of RAII).

Steps to reproduce

Sample code being reported below:

void MyClass::my_method() {
    // Some code that does not require mutex
    {
        std::lock_guard<std::mutex> lock(m_mutex);
        // Do something requiring a lock
    }
    // Some other code that does not require mutex
}

Question

To me this sounds like a false positive, but there might be reasons that I am not aware of to reject this usage? Could you please elaborate on why it is reported in that case?

Some people debating this topic on StackOverflow.

If this case is acceptable, then is it possible to update this rule so that it will not report it any more?

[vlad.loghin (Vlad-Andrei Loghin)]

Hey @FlorentP42, great to see you again!

Let me begin by thanking you for taking the time to provide your feedback! I believe that cpp:S1199 would be a minor annoyance for most people using unnamed scopes to implement RAII for critical sections.

To answer your question, cpp:S1199 is working as intended, and your example is not a false positive. Unnamed scopes are a code smell since they affect the code’s readability. In this specific case, it can be treated as a hotspot for readability, and there are multiple ways to approach this.

One easy solution to this issue is by extracting the code inside the unnamed scope into a separate function. Refactoring will improve the readability since it will state the purpose of the critical section via the function name. Moreover, it often comes with no performance impact (due to inlining).

Your example can be adapted quickly to satisfy cpp:S1199:

void MyClass::my_method() {
  // Some code that does not require mutex
  do_something_requiring_a_lock();
  // Some other code that does not require mutex
}

void MyClass::do_something_requiring_a_lock() {
  std::lock_guard<std::mutex> lock(m_mutex);
  // Do something requiring a lock
}

By doing this, both cpp:S1199 (Nested code blocks should not be used) and cpp:S5506 (“try_lock”, “lock” and “unlock” should not be directly used for mutexes) are satisfied.

This also prevents an issue mentioned in the StackOverflow topic you presented: protection of initializations. This way, you can initialize a value in the function, and copy elision will handle the return optimization.

However, this is far from a perfect solution. There are strong arguments for using unnamed scopes:

• You will lose the locality of the code
• You may have to pass many arguments, impacting readability
• It may just be too verbose.

Some might argue that you could use a lambda for the critical section, but that is a more cumbersome solution.

Therefore, another solution is to mark the issue as “won’t fix” in SonarCloud in the cases where the disadvantages of unnamed scopes outweigh the benefits.

Regardless, some projects heavily use unnamed scopes for mutex RAII. In this case, refactoring and marking the issues as “won’t fix” would be a time-consuming and painful endeavor. For this scenario, you can set up a custom Quality Profile for your project to disable this specific rule.
My advice, though, would be to keep using the default Quality Profile and use Clean as You Code to get rid of the issues in time.

I hope this answers your question, and let us know what you think!

[FlorentP42]

Thanks for taking the time to provide a detailed anwser. Sadly as I am getting these reports from the SonarCloud of a public repository I contribute to, so I do not have high enough rights on said repository to set any sort of “won’t fix” flag nor any custom profile for Sonarqube to use…

One workaround we discussed in our team and ended up choosing was to use dummy if() statement as a reason for the block to be there (using some already existing variable that we know the value for sure in the if() to make sure we always enter it) along with a comment to explain the situation.
This is not ideal solution in term of readability but at least it keeps the code in one place and avoids too much complexification like using sub-methods or lambda.