[clang-format] Allow empty loops on a single line.

[gedare]

Summary

Changes the AllowShortLoopsOnASingleLine from a boolean to a style option
consisting of three choices: Never, NonEmpty, and All. Never does not merge a
loop body back to the header's line. NonEmpty allows for merging a loop body
only if it has an expression. The new option is All, which will merge empty
loop bodies, i.e., a semi-colon.

The options of true and false are maintained for backward compatibility to mean
NonEmpty and Never, respectively.

See also Github Issue 61708.
https://github.com/llvm/llvm-project/issues/61708

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

595–599

[gedare]

Reorder traits and add comment about backward compatibility.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

[gedare]

Regenerate complete diff

[HazardyKnusperkeks]

Please wait for other opinions.

[owenpan]

Like while (a);, while (a) {} is also an empty loop, so NonEmpty is misleading if it excludes the former but not the latter. IMO we should just fix the bug without extending the option because any loop with a single-statement body is a short loop.

clang/unittests/Format/FormatTest.cpp

1444–1467 ↗

(On Diff #537859)

There should be no spaces between ) and ;.

[gedare]

In D154550#4526386, @owenpan wrote:

Like while (a);, while (a) {} is also an empty loop, so NonEmpty is misleading if it excludes the former but not the latter. IMO we should just fix the bug without extending the option because any loop with a single-statement body is a short loop.

Agreed, except that many style guides (notably, K&R, LLVM, and Google) treat these two cases differently. The loop with only a semi-colon is a special case, and is what I'm looking to support. K&R defines the semi-colon terminated for/while loop as a body composed of a null statement. I believe that { } is instead an empty body, as it does not parse as a statement, despite generating identical code. Maybe`NonNullStatement` is precise and better than NonEmpty? I will prepare that change, and fix the spacing before the ;.

Whether someone thinks collapsing bracketed bodies that are short should be supported is a separate problem, and even if so, it would require a similar style option change as I suggest to allow for backward compatibility. I'm not personally interested in that case, as the code base I work with uses the isolated semicolon.

[gedare]

Change option name from NonEmpty to NonNullStatement. Fix space before semi.

[owenpan]

In D154550#4529346, @gedare wrote:

In D154550#4526386, @owenpan wrote:

Like while (a);, while (a) {} is also an empty loop, so NonEmpty is misleading if it excludes the former but not the latter. IMO we should just fix the bug without extending the option because any loop with a single-statement body is a short loop.

Agreed, except that many style guides (notably, K&R, LLVM, and Google) treat these two cases differently.

LLVM doesn't merge short loops. Google uses {} instead of ; whereas AFAIK K&R does the opposite. I don't know of any major style that requires breaking before the null statement and merging the empty block.

[gedare]

This comment was removed by gedare.

[gedare]

In D154550#4562061, @owenpan wrote:

In D154550#4529346, @gedare wrote:

In D154550#4526386, @owenpan wrote:

Like while (a);, while (a) {} is also an empty loop, so NonEmpty is misleading if it excludes the former but not the latter. IMO we should just fix the bug without extending the option because any loop with a single-statement body is a short loop.

Agreed, except that many style guides (notably, K&R, LLVM, and Google) treat these two cases differently.

LLVM doesn't merge short loops. Google uses {} instead of ; whereas AFAIK K&R does the opposite. I don't know of any major style that requires breaking before the null statement and merging the empty block.

Got it. I can prepare a change to merge null as a short loop. I will get back to this in a couple weeks.

[gedare]

This is not needed due to https://github.com/llvm/llvm-project/pull/70768

clang/lib/Format/Format.cpp

static void enumeration(IO &IO, FormatStyle::ShortLambdaStyle &Value) {
     IO.enumCase(Value, "Inline", FormatStyle::SLS_Inline);
     IO.enumCase(Value, "All", FormatStyle::SLS_All);
     IO.enumCase(Value, "true", FormatStyle::SLS_All);
   }
 };
 
 template <> struct ScalarEnumerationTraits<FormatStyle::ShortWhileForLoopStyle> {
   static void enumeration(IO &IO, FormatStyle::ShortWhileForLoopStyle &Value) {
     IO.enumCase(Value, "Never", FormatStyle::SWFLS_Never);
-    IO.enumCase(Value, "false", FormatStyle::SWFLS_Never);
     IO.enumCase(Value, "NonEmpty", FormatStyle::SWFLS_NonEmpty);
     IO.enumCase(Value, "All", FormatStyle::SWFLS_All);
+
+    // For backward compatibility.
+    IO.enumCase(Value, "false", FormatStyle::SWFLS_Never);
     IO.enumCase(Value, "true", FormatStyle::SWFLS_NonEmpty);
   }
 };
 
 template <> struct ScalarEnumerationTraits<FormatStyle::SortIncludesOptions> {
   static void enumeration(IO &IO, FormatStyle::SortIncludesOptions &Value) {
     IO.enumCase(Value, "Never", FormatStyle::SI_Never);
     IO.enumCase(Value, "CaseInsensitive", FormatStyle::SI_CaseInsensitive);