[clang-format] [PR45338] AllowShortIfStatementsOnASingleLine: WithoutElse follows the documented behaviour

[Bouska]

Summary

According to clang-format documentation, the value WithoutElse of the option AllowShortIfStatementsOnASingleLine is expected to "put short ifs on the same line only if the else is not a compound statement". However, with the current code, and the option AllowShortBlocksOnASingleLine: Always, a short if block is always put on a single line whatever the following else statement is compound or not, and a short if statement is never put on a single line whatever the following else statement is compound or not. With this patch, the formatting is as documented.

Fixes #45338

If this patch fixes the simple example given on the documentation, the formatting or more complex code with else if and nested if (or loops) is not what I would expect from this option. Here are some formatted examples (with the options {BasedOnStyle: google, BreakBeforeBraces: Allman, AllowShortBlocksOnASingleLine: Always, AllowShortIfStatementsOnASingleLine: WithoutElse} and proposed patch):

int main()
{
  if (true) { function(); }

  if (true) function();

  if (true) { function(); }
  else
    function();

  if (true)
  {
    function();
  }
  else
  {
    function();
  }

  if (true) function();
  else
    function();

  if (true)
    function();
  else
  {
    function();
  }

  if (true) function();
  else if (false)
    function();
  else
  {
    function();
  }

  if (true) { function(); }
  else if (false)
    function();
  else
    function();

  if (true)
    function();
  else if (false)
  {
    function();
  }
  else
    function();

  if (true) function();
  else if (true)
    if (false) function();

  if (true) function();
  else if (true)
    if (false) { function(); }

  if (true) { function(); }
  else if (true)
    if (false) function();

  if (true) { function(); }
  else if (true)
    if (false) { function(); }
}

The first behaviour I would expect is that a short if statement is either never put in a short line if there is more than one else or only put on a single line when none of the else statement are compound. The second behaviour I would expect is that a nested if (or loop) is considered as a compound statement and prevents the is statement to be put in a single line.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

518

So I'd only expect this to get merged if it didn't have an else

[Bouska]

clang/unittests/Format/FormatTest.cpp

518

If it was just up to me, that would also be the behaviour I would expect but it doesn't match the current documentation... (and for Always, I would also expect the else if to be merged, but that's another subject)

[MyDeveloperDay]

This comment was removed by MyDeveloperDay.

[MyDeveloperDay]

Are all the cases from the summary contained in the unit tests?

[Bouska]

In D81808#2097732, @MyDeveloperDay wrote:

Are all the cases from the summary contained in the unit tests?

No, not all. Here is the detail:

int main()
{
  if (true) { function(); }  // Yes, FormatTests.cpp line 603

  if (true) function();  // Yes, FormatTests.cpp line 494

  if (true) { function(); }  // Yes, FormatTests.cpp line 661
  else
    function();

  if (true)  // Yes similar, FormatTests.cpp line 654
  {
    function();
  }
  else
  {
    function();
  }

  if (true) function();  // Yes, FormatTests.cpp line 514
  else
    function();

  if (true)  // Yes similar, FormatTests.cpp line 508 
    function();
  else
  {
    function();
  }

  if (true) function();  // No
  else if (false)
    function();
  else
  {
    function();
  }

  if (true) { function(); }  // No
  else if (false)
    function();
  else
    function();

  if (true)  // No
    function();
  else if (false)
  {
    function();
  }
  else
    function();

  if (true) function();  // No
  else if (true)
    if (false) function();

  if (true) function();  // No
  else if (true)
    if (false) { function(); }

  if (true) { function(); }  // No
  else if (true)
    if (false) function();

  if (true) { function(); }  // No
  else if (true)
    if (false) { function(); }
}

[MyDeveloperDay]

I think we should add all the other cases to ensure we don't break them down the road

[curdeius]

Wasn't this fixed/superseded by https://reviews.llvm.org/rG8d93d7ffedebc5f18dee22ba954d38a1d2d0affa? And so, the bug report might be closed (the old Always option should be avoided as it's a misnomer).

clang/lib/Format/UnwrappedLineFormatter.cpp

unsigned tryMergeSimpleControlStatement(
     if (Line.First->is(tok::kw_do) && !Line.Last->is(tok::kw_do))
       return 0;
     if (1 + I[1]->Last->TotalLength > Limit)
       return 0;
     if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for, tok::kw_while,
                              TT_LineComment))
       return 0;
     // Only inline simple if's (no nested if or else), unless specified
-    if (Style.AllowShortIfStatementsOnASingleLine != FormatStyle::SIS_Always) {
+    if (Style.AllowShortIfStatementsOnASingleLine == FormatStyle::SIS_Never) {
       if (I + 2 != E && Line.startsWith(tok::kw_if) &&
           I[2]->First->is(tok::kw_else))
         return 0;
+    } else if (Style.AllowShortIfStatementsOnASingleLine ==
+               FormatStyle::SIS_WithoutElse) {
+      if (I + 2 != E && I + 3 != E && Line.startsWith(tok::kw_if) &&
+          I[2]->First->is(tok::kw_else) &&
+          (I[2]->Last->is(tok::l_brace) || I[3]->First->is(tok::l_brace)))
+        return 0;
     }
+
     return 1;
   }
 
   unsigned
   tryMergeShortCaseLabels(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
                           SmallVectorImpl<AnnotatedLine *>::const_iterator E,
                           unsigned Limit) {
     if (Limit == 0 || I + 1 == E ||
if (Line.Last->is(tok::l_brace)) {
         Tok = I[2]->First;
         if (Tok->isNot(tok::r_brace))
           return 0;
 
         // Don't merge "if (a) { .. } else {".
         if (Tok->Next && Tok->Next->is(tok::kw_else))
           return 0;
 
+        // Don't merge a if statement if followed by a compound else
+        if (Style.AllowShortIfStatementsOnASingleLine ==
+                FormatStyle::SIS_WithoutElse &&
+            I + 3 != E && I[3]->First->is(tok::kw_else) &&
+            (I[3]->Last->is(tok::l_brace) ||
+             (I + 4 != E && I[4]->First->is(tok::l_brace))))
+          return 0;
+
         // Don't merge a trailing multi-line control statement block like:
         // } else if (foo &&
         //            bar)
         // { <-- current Line
         //   baz();
         // }
         if (Line.First == Line.Last &&
             Style.BraceWrapping.AfterControlStatement ==

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
   AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
       FormatStyle::SIS_WithoutElse;
   verifyFormat("if (a)\n"
                "  f();\n"
                "else {\n"
                "  g();\n"
                "}",
                AllowsMergedIf);
-  verifyFormat("if (a)\n"
-               "  f();\n"
+  verifyFormat("if (a) f();\n"
                "else\n"
                "  g();\n",
                AllowsMergedIf);
 
   AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
 
   verifyFormat("if (a) f();\n"
                "else {\n"
                "  g();\n"
                "}",
                AllowsMergedIf);
   verifyFormat("if (a) f();\n"
verifyFormat("if (true) {\n"
                "}",
                AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
                "  f();\n"
                "} else {\n"
                "  f();\n"
                "}",
                AllowSimpleBracedStatements);
+  verifyFormat("if (true) {\n"
+               "  f();\n"
+               "} else\n"
+               "  f();\n",
+               AllowSimpleBracedStatements);
 
   verifyFormat("struct A2 {\n"
                "  int X;\n"
                "};",
                AllowSimpleBracedStatements);
   verifyFormat("typedef struct A2 {\n"
                "  int X;\n"
                "} A2_t;",
                AllowSimpleBracedStatements);
   verifyFormat("template <int> struct A2 {\n"
                "  struct B {};\n"
                "};",
                AllowSimpleBracedStatements);
 
+  AllowSimpleBracedStatements.BraceWrapping.BeforeElse = true;
+
+  verifyFormat("if (true) {\n"
+               "  f();\n"
+               "}\n"
+               "else {\n"
+               "  f();\n"
+               "}",
+               AllowSimpleBracedStatements);
+  verifyFormat("if (true) { f(); }\n"
+               "else\n"
+               "  f();\n",
+               AllowSimpleBracedStatements);
+
+  AllowSimpleBracedStatements.BraceWrapping.BeforeElse = false;
+
   AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
       FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
                "  f();\n"
                "}",
                AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"