AllowShortLoopsOnASingleLine does not work on trivial loops

[Quuxplusone]

Bugzilla Link	PR41752
Status	CONFIRMED
Importance	P enhancement
Reported by	Andrew Aksyonoff (shodan@shodan.ru)
Reported on	2019-05-05 06:34:58 -0700
Last modified on	2019-12-01 02:50:07 -0800
Version	8.0
Hardware	PC Windows NT
CC	djasper@google.com, klimek@google.com, llvm-bugs@lists.llvm.org, mydeveloperday@gmail.com, pablomg+llvm@eskapa.be
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

.clang-format contents:

Language: Cpp
BasedOnStyle: LLVM
BreakBeforeBraces: Allman
AllowShortLoopsOnASingleLine: true

Input file:

void Foo()
{
  while (*p++);
}

Expected output: same as input.

Actual output: semicolon gets detached.

void Foo()
{
  while (*p++)
    ;
}

[Quuxplusone]

This is relatively easily fixed by removing the tok::semi clause from inside
tryMergeSimpleControlStatement

    if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for, tok::kw_while,

                             TT_LineComment))
      return 0;

But this then break test which specifically assert for this behaviour. (even
with allow short loops and short ifs)

  verifyFormat("if (a)\n"
               "  ;",
               AllowsMergedIf);

  AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;

  verifyFormat("for (;;)\n"
               "  ;",
               AllowsMergedLoops);

  verifyFormat("while (true)\n"
               "  ;",
               AllowsMergedLoops);

I'm unsure what to think about this, in terms of presenting

  while (*p++)
    ;
  while (*p)
    ;
  while (*++p)
    ;
  while (true)
    ;
  while (false)
    ;

vs

  while (*p++) ;
  while (*p) ;
  while (*++p) ;
  while (true) ;
  while (false) ;

All I can say is if AllowShortLoopsOnASingleLine is set to false you get the
same output, (which kind of feels wrong)

  while (*p++)
    ;
  while (*p)
    ;
  while (*++p)
    ;
  while (true)
    ;
  while (false)
    ;

Any thoughts? justification etc..?

[Quuxplusone]

Seems a pretty funny rule that differentiates; on its own

AllowShortLoopsOnASingleLine: true

  while (false) continue;
  while (false) return;
  while (false) break;
  while (false) --i;
  while (false) i++;
  while (false)
    ;

AllowShortLoopsOnASingleLine: false

  while (false)
    continue;
  while (false)
    return;
  while (false)
    break;
  while (false)
    --i;
  while (false)
    i++;
  while (false)
    ;

[Quuxplusone]

From the google style guide.....

Quote:
"
Empty loop bodies should use either an empty pair of braces or continue with no
braces, rather than a single semicolon.

while (condition) {
  // Repeat test until it returns false.
}
for (int i = 0; i < kSomeNumber; ++i) {}  // Good - one newline is also OK.
while (condition) continue;  // Good - continue indicates no logic.
while (condition);  // Bad - looks like part of do/while loop.
"

So whilst the google style guide obviously recognizes and encourages the use of
"AllowShortLoopsOnASingleLine:true"

"while (condition) continue;"

They also recognize that the style, should someone use a ";" to represent a
very small loop, they'd be expecting to see that on the same line.

"while (condition);"

The ONLY caveat to that is that they may be using the ";" on a newline to make
the ; obvious (as this can be a source of bugs)

However its not the role of clang-format to find bugs though formatting.

There are 2 choices

1) add a new style AllowEmptyLoopsOnASingleLine:true  (which really feels like
overkill to me)
2) make the single ";" honour AllowShortLoopsOnASingleLine

[Quuxplusone]

As point out by MyDeveloperDay, the issue is also present for if with
AllowShortIfStatementsOnASingleLine: WithoutElse/Always.

if (true) ;
becomes
if (true)
  ;

(I use if (true) {} with AllowShortBlocksOnASingleLine: Empty as a workaround)