Page MenuHomePhabricator

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

Authored by MyDeveloperDay on May 18 2020, 10:35 AM.

Details

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]] {
  // ...
}

Diff Detail

Event Timeline

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 added a comment.EditedMay 19 2020, 1:00 AM

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 accepted this revision.May 19 2020, 6:23 AM

LGTM assuming CI tests pass.

This revision is now accepted and ready to land.May 19 2020, 6:23 AM
This revision was automatically updated to reflect the committed changes.

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?

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 added a comment.EditedMay 25 2020, 10:04 AM

maybe for now we just do

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

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.