[clang-tidy] [PR50069] readability-braces-around-statements doesn't work well with [[likely]] [[unlikely]]

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=50069

When clang-tidy sees:

if (true) [[unlikely]] {
    ...
}

It thinks the braces are missing and add them again.

if (true)  { [[unlikely]] {
    ...
  }
}

This revision aims to prevent that incorrect code generation

[aaron.ballman]

Thank you for working on this!

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements-attributes.cpp

7–14

Can you also add tests that show we do the correct thing for:

if (b) [[likely]]
  return;
if (b) [[unlikely]]
  return;

where there were no braces involved? We should ensure that it wants to insert the braces after the attribute.

[MyDeveloperDay]

In D105479#2862819, @aaron.ballman wrote:

Thank you for working on this!

So I looked into do what you suggested originally, but it became a little more involved, partially because you have to look inside the AttrbutedStmt but then getting the {} to cuddle the statement and not the attributes (and this means a lot more code has to be changed because you have to move the position of S to that statement so that you determine the correct place to put replacements, (and then this got way beyond my pay grade!)

So for this patch I was limiting it just to fixing the issue (that was actually incorrectly raised against #clang-format , hence why I'm here!)

I don't personally use the [[likely]] attributes yet so I wasn't completely sure of all the places it COULD be added.

For the case you raised I also don't believe it will do the correct thing, https://godbolt.org/z/xb1qrTe3j infact it will generate:

if (b) {
   [[likely]]
   return;
}

Use https://godbolt.org/z/YzrKzKoTe then add -fix to the clang-tidy Arguments (it will destroy your original source)

Which I wasn't sure if this is then still valid syntax? Although it may compile I couldn't really tell if it was then being ignored.

[aaron.ballman]

In D105479#2863819, @MyDeveloperDay wrote:

In D105479#2862819, @aaron.ballman wrote:

Thank you for working on this!

So I looked into do what you suggested originally, but it became a little more involved, partially because you have to look inside the AttrbutedStmt but then getting the {} to cuddle the statement and not the attributes (and this means a lot more code has to be changed because you have to move the position of S to that statement so that you determine the correct place to put replacements, (and then this got way beyond my pay grade!)

Hmm, my naïve approach would be to do:

// If the statement is an attributed one, we should be worried about its non-attributed substatement.
while (const auto *AS = dyn_cast<AttributedStmt>(S))
  S = AS->getSubStmt();

// 1) If there's a corresponding "else" or "while", the check inserts "} "
// right before that token.
// 2) If there's a multi-line block comment starting on the same line after
// the location we're inserting the closing brace at, or there's a non-comment
// token, the check inserts "\n}" right before that token.
// 3) Otherwise the check finds the end of line (possibly after some block or
// line comments) and inserts "\n}" right before that EOL.
if (!S || isa<CompoundStmt>(S)) {
  // Already inside braces.
  return false;
}

Does that approach fail for reasons I'm not thinking of?

So for this patch I was limiting it just to fixing the issue (that was actually incorrectly raised against #clang-format , hence why I'm here!)

The trouble is, it introduces new issues, so it moves the bug around rather than fixing it. Though it does improve the situation for when you use a compound statement, so from that angle, it's at least a minor improvement.

I don't personally use the [[likely]] attributes yet so I wasn't completely sure of all the places it COULD be added.

For the case you raised I also don't believe it will do the correct thing, https://godbolt.org/z/xb1qrTe3j infact it will generate:

if (b) {
   [[likely]]
   return;
}

Use https://godbolt.org/z/YzrKzKoTe then add -fix to the clang-tidy Arguments (it will destroy your original source)

Which I wasn't sure if this is then still valid syntax? Although it may compile I couldn't really tell if it was then being ignored.

It's valid syntax that doesn't do anything useful. It's saying "when you're within this compound statement, it's likely that you'll hit this return." rather than "this branch of the if statement is the likely branch". It's basically the same issue as the one you're trying to fix now. While this patch does make some forward progress, I'm hopeful that we can fix both issues at once and be done with it. If my suggestion above doesn't pan out, then I think we can land this as being incremental progress, but we should still add the test case with a FIXME comment and open a bug report about the remaining issue.

[MyDeveloperDay]

Address the situation where we don't have braces after the [[likely]] on an if

$ ninja check-clang-tools
-- Testing: 976 tests, 16 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..

Testing Time: 128.04s
  Unsupported      :   9
  Passed           : 965
  Expectedly Failed:   2

[aaron.ballman]

LGTM, thank you for the fix!

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

bool BracesAroundStatementsCheck::checkStmt(
     SourceLocation InitialLoc, SourceLocation EndLocHint) {
   // 1) If there's a corresponding "else" or "while", the check inserts "} "
   // right before that token.
   // 2) If there's a multi-line block comment starting on the same line after
   // the location we're inserting the closing brace at, or there's a non-comment
   // token, the check inserts "\n}" right before that token.
   // 3) Otherwise the check finds the end of line (possibly after some block or
   // line comments) and inserts "\n}" right before that EOL.
-  if (!S || isa<CompoundStmt>(S)) {
+  if (!S || isa<CompoundStmt>(S) || isa<AttributedStmt>(S)) {
     // Already inside braces.
     return false;
   }
 
   if (!InitialLoc.isValid())
     return false;
   const SourceManager &SM = *Result.SourceManager;
   const ASTContext *Context = Result.Context;

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements-attributes.cpp

+// RUN: %check_clang_tidy  -std=c++20-or-later %s readability-braces-around-statements %t
+
+void test(bool b) {
+  if (b) {
+    return;
+  }
+  if (b) [[likely]] {
+    // CHECK-FIXES-NOT: if (b) { {{[[][[]}}likely{{[]][]]}} {
+    return;
+  }
+  if (b) [[unlikely]] {
+    // CHECK-FIXES-NOT: if (b) { {{[[][[]}}unlikely{{[]][]]}} {
+    return;
+  }
+}