[clang-format] [PR25010] Extend AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay]

Summary

An addendum to D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present following discussions with submitter of https://bugs.llvm.org/show_bug.cgi?id=25010

it didn't make sense that, we could only do short if and not short else if and short else if they didn't have compound statments

if (true) return 0;

if (true) return 0;
else return 1;

if (true) return 0;
else if (false) return 1;

if (true) return 0;
else if (false) return 1;
else  return 2;

if (true) return 0;
else if (false) return 1;
else {
  return 2;
}

The following revision extends the capability form D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present to allow that if the correct setting is provided

existing true/false still honor the original meaning of AllowShortIfStatementsOnASingleLine

but the additional options now allow more fine grained control over how else/if and else should/could be handled.

[klimek]

clang/docs/ClangFormatStyleOptions.rst

401

I'd try to make this either unconditionally what we do, or decide against doing it.

clang/include/clang/Format/Format.h

263–264

This seems weird - why would we want this?

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

401

see note before, this is really about maintaining compatibility, I don't want to make the assumption that everyone likes

if (x) return 1;
else if (x) return 2;
else return 3;

There could easily be users who want this.

if (x) return 1;
else if (x) return 2;
else 
   return 3;

I don't think it over complicates it, but it keeps the flexibility. The name of the option may not be the best 'suggestions welcome'

clang/include/clang/Format/Format.h

263–264

This is a compatiblity case for the previous "true" as it was before

Before

if (x) 
   return 1;
else
   return 2;

would be transformed to

if (x) return 1;
else
   return 2;

but

if (x) 
   return 1;
else {
   return 2;
}

would not be transformed at all

[rdwampler]

clang/include/clang/Format/Format.h

264

These comments can be used to auto generate the corresponding sections in clang/docs/ClangFormatStyleOptions.rst using clang/docs/tools/dump_format_style.py, so they should probably be kept in sync.

[krasimir]

Generally I'm against introducing new style flags or flag options unless a supported style requires it.
IMO this increases maintenance burden and can quickly lead to a big space of style flags that contain tricky incompatibilities.

[krasimir]

clang/docs/ClangFormatStyleOptions.rst

411

nit: I prefer the term if statement instead of if/else statement.

clang/include/clang/Format/Format.h

263

ditto nit: replace If Else statements with If statements.

272

this sentence does not parse. Consider rewriting.

clang/lib/Format/UnwrappedLineFormatter.cpp

368

nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.

422

nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.

430

nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.

Or extract a function that provides this functionality here and in other places.

clang/unittests/Format/FormatTest.cpp

520

I don't understand why is this preferred over:

if (a) f();
else
  if (b) g();

Isn't syntactically the nested if (b) g(); a compound statement? I might be wrong.

[klimek]

clang/docs/ClangFormatStyleOptions.rst

401

The second one seems simply broken. I think fixing this is great, but keeping the broken version seems odd :)

clang/unittests/Format/FormatTest.cpp

520

There's no compound statement, but the else-stmt is another if-stmt. That said, I don't think this is about what structure the C++ standard has (as that's not what clang-format understands anyway), but whether the structure we put in helps readability. In this case, why does the irregular break before the if help readability? It seems like we're having a break for no good reason.

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
          return;
 
        if (a)
          return;
        else {
          return;
        }
 
+  * ``SIS_AlwaysNoElse`` (in configuration: ``AlwaysNoElse``)
+    Allow short if/else if statements even if the else is a compound statement.
+
+    .. code-block:: c++
+
+       if (a) return;
+       else {
+          return;
+       }
+
   * ``SIS_Always`` (in configuration: ``Always``)
-    Allow short if statements even if the else is a compound statement.
+    Allow short if/else if/else statements when they are not compound statements.
 
     .. code-block:: c++
 
        if (a) return;
+       else return;
+
+       if (a) return;
+       else if (b) return;
+       else return;
+       
+       if (a) return;
        else {
           return;
        }
 
+       if (a) return;
+       else if (b) return;
+       else {
+          return;
+       }
+
+       if (a) return;
+       else if (b) return;
+       else return;
+
 **AllowShortLoopsOnASingleLine** (``bool``)
   If ``true``, ``while (true) continue;`` can be put on a single
   line.
 
 **AlwaysBreakAfterDefinitionReturnType** (``DefinitionReturnTypeBreakingStyle``)
   The function definition return type breaking style to use.  This
   option is **deprecated** and is retained for backwards compatibility.
 

clang/include/clang/Format/Format.h

enum ShortIfStyle {
     /// Without else put short ifs on the same line only if
     /// the else is not a compound statement.
     /// \code
     ///   if (a) return;
     ///   else
     ///     return;
     /// \endcode
     SIS_WithoutElse,
-    /// Always put short ifs on the same line if
-    /// the else is not a compound statement or not.
+    /// If Else statements have no braces don't put them
+    /// on the same line.
+    /// \code
+    ///   if (a) return;
+    ///   else
+    ///     return;
+    /// \endcode
+    SIS_AlwaysNoElse,
+    /// Always put short if/else/else if on the same line if
+    /// the not compound statement.
     /// \code
     ///   if (a) return;
     ///   else {
     ///     return;
     ///   }
+    ///   if (a) return;
+    ///   else if (b) return;
+    ///   else {
+    ///     return;
+    ///   }
+    ///
+    ///   if (a) return;
+    ///   else if (b) return;
+    ///   else return;
+    ///
+    ///   if (a) return;
+    ///   else return;
     /// \endcode
     SIS_Always,
   };
 
   /// If ``true``, ``if (a) return;`` can be put on a single line.
   ShortIfStyle AllowShortIfStatementsOnASingleLine;
 
   /// If ``true``, ``while (true) continue;`` can be put on a single

clang/lib/Format/Format.cpp

static void enumeration(IO &IO, FormatStyle::ShortFunctionStyle &Value) {
     IO.enumCase(Value, "Empty", FormatStyle::SFS_Empty);
   }
 };
 
 template <> struct ScalarEnumerationTraits<FormatStyle::ShortIfStyle> {
   static void enumeration(IO &IO, FormatStyle::ShortIfStyle &Value) {
     IO.enumCase(Value, "Never", FormatStyle::SIS_Never);
     IO.enumCase(Value, "Always", FormatStyle::SIS_Always);
+    IO.enumCase(Value, "AlwaysNoElse", FormatStyle::SIS_AlwaysNoElse);
     IO.enumCase(Value, "WithoutElse", FormatStyle::SIS_WithoutElse);
 
     // For backward compatibility.
     IO.enumCase(Value, "false", FormatStyle::SIS_Never);
     IO.enumCase(Value, "true", FormatStyle::SIS_WithoutElse);
   }
 };
 

clang/lib/Format/UnwrappedLineFormatter.cpp

if (I[1]->First->is(TT_FunctionLBrace) &&
       }
       return MergedLines;
     }
     if (TheLine->First->is(tok::kw_if)) {
       return Style.AllowShortIfStatementsOnASingleLine
                  ? tryMergeSimpleControlStatement(I, E, Limit)
                  : 0;
     }
+    if (TheLine->First->startsSequence(tok::kw_else, tok::kw_if)) {
+      return (Style.AllowShortIfStatementsOnASingleLine >=
+              FormatStyle::SIS_AlwaysNoElse)
+                 ? tryMergeSimpleControlStatement(I, E, Limit)
+                 : 0;
+    }
+    if (TheLine->First->is(tok::kw_else)) {
+      return Style.AllowShortIfStatementsOnASingleLine ==
+                     FormatStyle::SIS_Always
+                 ? tryMergeSimpleControlStatement(I, E, Limit)
+                 : 0;
+    }
     if (TheLine->First->isOneOf(tok::kw_for, tok::kw_while)) {
       return Style.AllowShortLoopsOnASingleLine
                  ? tryMergeSimpleControlStatement(I, E, Limit)
                  : 0;
     }
     if (TheLine->First->isOneOf(tok::kw_case, tok::kw_default)) {
       return Style.AllowShortCaseLabelsOnASingleLine
                  ? tryMergeShortCaseLabels(I, E, Limit)
unsigned tryMergeSimpleControlStatement(
     if (Style.BraceWrapping.AfterControlStatement &&
         (I[1]->First->is(tok::l_brace) && !Style.AllowShortBlocksOnASingleLine))
       return 0;
     if (I[1]->InPPDirective != (*I)->InPPDirective ||
         (I[1]->InPPDirective && I[1]->First->HasUnescapedNewline))
       return 0;
     Limit = limitConsideringMacros(I + 1, E, Limit);
     AnnotatedLine &Line = **I;
-    if (Line.Last->isNot(tok::r_paren))
+    if (Line.Last->isNot(tok::r_paren) &&
+        Style.AllowShortIfStatementsOnASingleLine < FormatStyle::SIS_Always)
       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_AlwaysNoElse) {
       if (I + 2 != E && Line.startsWith(tok::kw_if) &&
           I[2]->First->is(tok::kw_else))
         return 0;
     }
     return 1;
   }
 
   unsigned
tryMergeSimpleBlock(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
     // FIXME: If an option to allow short exception handling clauses on a single
     // line is added, change this to not return for @try and friends.
     if (Style.Language != FormatStyle::LK_Java &&
         Line.First->isOneOf(tok::at, tok::minus, tok::plus))
       return 0;
 
     // Check that the current line allows merging. This depends on whether we
     // are in a control flow statements as well as several style flags.
-    if (Line.First->isOneOf(tok::kw_else, tok::kw_case) ||
+    if (Line.First->is(tok::kw_case) ||
         (Line.First->Next && Line.First->Next->is(tok::kw_else)))
       return 0;
     // default: in switch statement
     if (Line.First->is(tok::kw_default)) {
       const FormatToken *Tok = Line.First->getNextNonComment();
       if (Tok && Tok->is(tok::colon))
         return 0;
     }
-    if (Line.First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_do, tok::kw_try,
-                            tok::kw___try, tok::kw_catch, tok::kw___finally,
-                            tok::kw_for, tok::r_brace, Keywords.kw___except)) {
+    if (Line.First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while, tok::kw_do,
+                            tok::kw_try, tok::kw___try, tok::kw_catch,
+                            tok::kw___finally, tok::kw_for, tok::r_brace,
+                            Keywords.kw___except)) {
       if (!Style.AllowShortBlocksOnASingleLine)
         return 0;
       // Don't merge when we can't except the case when
       // the control statement block is empty
       if (!Style.AllowShortIfStatementsOnASingleLine &&
           Line.startsWith(tok::kw_if) &&
           !Style.BraceWrapping.AfterControlStatement &&
           !I[1]->First->is(tok::r_brace))
         return 0;
       if (!Style.AllowShortIfStatementsOnASingleLine &&
+          Line.startsWith(tok::kw_else) &&
+          !Style.BraceWrapping.AfterControlStatement &&
+          !I[1]->First->is(tok::r_brace))
+        return 0;
+      if (!Style.AllowShortIfStatementsOnASingleLine &&
           Line.startsWith(tok::kw_if) &&
           Style.BraceWrapping.AfterControlStatement && I + 2 != E &&
           !I[2]->First->is(tok::r_brace))
         return 0;
       if (!Style.AllowShortLoopsOnASingleLine &&
           Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for) &&
           !Style.BraceWrapping.AfterControlStatement &&
           !I[1]->First->is(tok::r_brace))

clang/unittests/Format/FormatTest.cpp

verifyFormat("if (a)\n"
                "}",
                AllowsMergedIf);
   verifyFormat("if (a)\n"
                "  f();\n"
                "else\n"
                "  g();\n",
                AllowsMergedIf);
 
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_AlwaysNoElse;
+
+  verifyFormat("if (a) f();\n"
+               "else {\n"
+               "  g();\n"
+               "}",
+               AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+               "else\n"
+               "  g();\n",
+               AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+               "else if (b) g();\n",
+               AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+               "else if (b) g();\n"
+               "else\n"
+               "  h();\n",
+               AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+               "else {\n"
+               "  if (a) f();\n"
+               "  else {\n"
+               "    g();\n"
+               "  }\n"
+               "  g();\n"
+               "}",
+               AllowsMergedIf);
+
   AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
 
   verifyFormat("if (a) f();\n"
                "else {\n"
                "  g();\n"
                "}",
                AllowsMergedIf);
   verifyFormat("if (a) f();\n"
                "else {\n"
                "  if (a) f();\n"
                "  else {\n"
                "    g();\n"
                "  }\n"
                "  g();\n"
                "}",
                AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+               "else g();\n",
+               AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+               "else if (b) g();\n",
+               AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+               "else if (b) g();\n"
+               "else h();\n",
+               AllowsMergedIf);
 }
 
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
   verifyFormat("while (true) continue;", AllowsMergedLoops);
   verifyFormat("for (;;) continue;", AllowsMergedLoops);
   verifyFormat("for (int &v : vec) v *= 2;", AllowsMergedLoops);