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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D105479#2863819 <https://reviews.llvm.org/D105479#2863819>, @MyDeveloperDay wrote:

> In D105479#2862819 <https://reviews.llvm.org/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 <https://reviews.llvm.org/tag/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105479/new/

https://reviews.llvm.org/D105479