[clang-format] @lefticus just taught the world how to use [[unlikely]] but we forgot to teach clang-format

[MyDeveloperDay]

Summary

https://twitter.com/lefticus/status/1262392152950288384?s=20

Jason Turner's (@lefticus) most recent C++ weekly explains the usage of [[likely]] and [[unlikely]] in an 'if/else' context in C++ 20

clang-format leaves the code a little messy afterwards..

if (argc > 5)
  [[unlikely]] {
    // ...
  }
else if (argc < 0)
  [[likely]] {
    // ...
  }
else
  [[likely]] {
    // ...
  }

try to improve the situation

if (argc > 5) [[unlikely]] {
  // ...
} else if (argc < 0) [[likely]] {
  // ...
} else [[likely]] {
  // ...
}

[MyDeveloperDay]

rebase

[JakeMerdichAMD]

This is a great improvement in readability. I think this will get in here before it does for clang proper :D

Likely/unlikely also seem to be supported on while, do-while, and for loops (case statements too, but that's not relevant). Should we also apply identical logic there? Far less useful/common than plain if's so it's not a blocker, but it's best to be consistent and I can imagine some decent use cases.

[MyDeveloperDay]

In D80144#2042926, @JakeMerdichAMD wrote:

This is a great improvement in readability. I think this will get in here before it does for clang proper :D

Likely/unlikely also seem to be supported on while, do-while, and for loops (case statements too, but that's not relevant). Should we also apply identical logic there? Far less useful/common than plain if's so it's not a blocker, but it's best to be consistent and I can imagine some decent use cases.

I struggled to find decent examples, cppreference.org and the standard is very thin on the ground. We should handle those cases but I feel maybe small increments are better as its not common code.

[JakeMerdichAMD]

LGTM assuming CI tests pass.

[krasimir]

Unfortunately this introduced a bad regression with ObjC method expressions in if bodies:

if (argc > 5) [obj func:arg];

is now formatted as:

if (argc > 5)[obj func:arg]
  ;

It seems that at this stage of processing it would be non-trivial to distinguish between general annotations and Objective-C call expressions; the general heuristics are at a later stage of clang-format's pipeline, in TokenAnnotator: https://github.com/llvm/llvm-project/blob/7c298c104bfe725d4315926a656263e8a5ac3054/clang/lib/Format/TokenAnnotator.cpp#L174.

The problem is that these two cases fundamentally require look ahead: we need to keep [[likely]] on the same unwrapped line as the if (argc > 5), but we need to put the [pbj func:arg] on a new unwrapped line.

Maybe we can try manually parsing forward looking for a [[identifier]] pattern, and if we don't find it, reset the token position, reset the token sequence to the point before the first left square brace; similarly to https://github.com/llvm/llvm-project/blob/7c298c104bfe725d4315926a656263e8a5ac3054/clang/lib/Format/UnwrappedLineParser.cpp#L1496.

I know that we didn't have a test for this in ObjC land, but the regression seems severe enough to revert this, add a regression test and work on adding if [[likely]]/[[unlikely]] support compatible with ObjC method expressions as a follow-up.

WDYT?

[MyDeveloperDay]

I know that we didn't have a test for this in ObjC land, but the regression seems severe enough to revert this

Sure go ahead, that's the process.

[MyDeveloperDay]

maybe for now we just do

if (FormatTok->is(tok::l_square) && !Style.isObjC())
    parseSquare();

[krasimir]

In D80144#2053543, @MyDeveloperDay wrote:

maybe for now we just do

if (FormatTok->is(tok::l_square) && !Style.isObjC())
    parseSquare();

I tried this, however FormatTest.cpp's verifyFormat is picky and explicitly checks both Cpp and ObjC styles return the same output (because ObjC++ is a thing):
https://github.com/llvm/llvm-project/blob/6f802ec4333cc1227bb37e258a81e9a588f964dc/clang/unittests/Format/FormatTest.cpp#L74
We can of course not use verifyFormat for these test cases, but this is a symptom that this may be the wrong approach.

[MyDeveloperDay]

Will this help? D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[krasimir]

In D80144#2054451, @MyDeveloperDay wrote:

Will this help? D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

This is awesome! Thank you! I'll switch to commenting there.

[Roelio81]

This change also has impact which was undesired on our code base as it now considers all attributes to be "bound" to the if-expression rather than to the if-body.
For example, we had this in our code base

if (condition)
   [[maybe_unused]] auto result = x->setValue(0.0);

This became the following after upgrading from clang 10:

if (condition) [[maybe_unused]] 
   auto result = x->setValue(0.0);

The [[maybe_unused]] is actually referring to the result variable, hence I think it makes more sense to have it on the second line.
We worked around the issue by adding curly braces, i.e.,

if (condition)
{
   [[maybe_unused]] auto result = x->setValue(0.0);
}

Edit: this one seems to be logged already: https://github.com/llvm/llvm-project/issues/59235

clang/lib/Format/UnwrappedLineParser.cpp

 
 void UnwrappedLineParser::parseIfThenElse() {
   assert(FormatTok->Tok.is(tok::kw_if) && "'if' expected");
   nextToken();
   if (FormatTok->Tok.isOneOf(tok::kw_constexpr, tok::identifier))
     nextToken();
   if (FormatTok->Tok.is(tok::l_paren))
     parseParens();
+  // handle [[likely]] / [[unlikely]]
+  if (FormatTok->is(tok::l_square))
+    parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     if (Style.BraceWrapping.BeforeElse)
       addUnwrappedLine();
     else
       NeedsUnwrappedLine = true;
   } else {
     addUnwrappedLine();
     ++Line->Level;
     parseStructuralElement();
     --Line->Level;
   }
   if (FormatTok->Tok.is(tok::kw_else)) {
     nextToken();
+    // handle [[likely]] / [[unlikely]]
+    if (FormatTok->is(tok::l_square))
+      parseSquare();
     if (FormatTok->Tok.is(tok::l_brace)) {
       CompoundStatementIndenter Indenter(this, Style, Line->Level);
       parseBlock(/*MustBeDeclaration=*/false);
       addUnwrappedLine();
     } else if (FormatTok->Tok.is(tok::kw_if)) {
       parseIfThenElse();
     } else {
       addUnwrappedLine();

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ(
              "int j;\n"
              "} // namespace\n"
              "  // "
              "would::it::save::you::a::lot::of::time::if_::i::just::gave::up::"
              "and_::went::mad::now",
              Style));
 }
 
+TEST_F(FormatTest, LikelyUnlikely) {
+  FormatStyle Style = getLLVMStyle();
+
+  verifyFormat("if (argc > 5) [[unlikely]] {\n"
+               "  return 29;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (argc > 5) [[likely]] {\n"
+               "  return 29;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (argc > 5) [[unlikely]] {\n"
+               "  return 29;\n"
+               "} else [[likely]] {\n"
+               "  return 42;\n"
+               "}\n",
+               Style);
+
+  verifyFormat("if (argc > 5) [[unlikely]] {\n"
+               "  return 29;\n"
+               "} else if (argc > 10) [[likely]] {\n"
+               "  return 99;\n"
+               "} else {\n"
+               "  return 42;\n"
+               "}\n",
+               Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang