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

[MyDeveloperDay]

Summary

Addressing: PR25010 - https://bugs.llvm.org/show_bug.cgi?id=25010

Code like:

if(true) var++;
else  {
    var--;
}

is reformatted to be

if (true)
  var++;
else {
  var--;
}

Even when AllowShortIfStatementsOnASingleLine is true

The following revision comes from a +1'd suggestion in the PR to support AllowShortIfElseStatementsOnASingleLine

This suppresses the clause prevents the merging of the if when there is a compound else

[MyDeveloperDay]

Add missing Format.h from the review

[Higuoxing]

clang/include/clang/Format/Format.h

247

Small nit:

AllowShortIfIfStatementsOnASingleLine
            ^

[MyDeveloperDay]

Fix spelling typo in documentation and comment

[reuk]

Does it make sense to allow AllowShortIfElseStatementsOnASingleLine to be enabled when AllowShortIfStatementsOnASingleLine is not?

Perhaps it would be better to have AllowShortIfStatementsOnASingleLine be represented by an enum with values Never, WithoutElse, Always. The old true/false values from config files prior to this change would need to be made to map to the appropriate enum keys.

[MyDeveloperDay]

In D59087#1423942, @reuk wrote:

Does it make sense to allow AllowShortIfElseStatementsOnASingleLine to be enabled when AllowShortIfStatementsOnASingleLine is not?

Perhaps it would be better to have AllowShortIfStatementsOnASingleLine be represented by an enum with values Never, WithoutElse, Always. The old true/false values from config files prior to this change would need to be made to map to the appropriate enum keys.

Thats a good point...its probably more code but I think it might be a better approach

[MyDeveloperDay]

Address review comments

• collapse two options into one

[reuk]

This looks much better now, thanks for taking another look! I've flagged some formatting/spelling nits, and I think the tests/docs could be a little more explicit about the behaviour of WithoutElse. Other than that, looks great!

clang/docs/ClangFormatStyleOptions.rst

370

Missing .

371

I think an example of the generated formatting *with* a compound else statement would be useful here too, in addition to the existing example.

375

For consistency, these descriptions should end with periods .

376

compounf -> compound

376

Missing .

clang/include/clang/Format/Format.h

264

Missing .

clang/lib/Format/UnwrappedLineFormatter.cpp

417

Has this line been clang-formatted? I'd expect spaces around the binop

clang/unittests/Format/FormatTest.cpp

494

I think having a test here which shows the expected behaviour for non-compound else statements with WithoutElse would be helpful, as in the documentation.

[MyDeveloperDay]

Address review comment

• add document and comment spelling and punctuation
• add additional unit tests to show non compound else clause case

[reuk]

The code looks good now. There's just a few places left where the formatting looks a bit suspect (sorry for missing those last time). I think once that's fixed this will be good to go.

clang/unittests/Format/FormatTest.cpp

546

This line, along with some of the following changed lines, have more than 80 characters. Make sure to clang-format changes! Consider using git-clang-format to format just this patch.

[MyDeveloperDay]

run git clang-format

[reuk]

LGTM

[krasimir]

I believe there is no such thing as an "short else statement". The else is part of the if statement and if it is present, I don't consider the whole if statement short. As such, IMO the bug is invalid.

[klimek]

cfe/trunk/docs/ClangFormatStyleOptions.rst

384 ↗

(On Diff #190379)

Is that actually the status quo or is the current behavior missing?

[MyDeveloperDay]

In D59087#1456129, @krasimir wrote:

I believe there is no such thing as an "short else statement". The else is part of the if statement and if it is present, I don't consider the whole if statement short. As such, IMO the bug is invalid.

I understand your point, but that is just one viewpoint only.

The opinion of the person who logged the defect (and I'm not that user just someone who wants to address the defects), whether something is a bug or not is 100% about their perspective, (that doesn't mean we have to address everything)

The question is does the change to the option hurt anyone who doesn't care? Is it useful the to the user requesting it? Does it significantly impact our ability to maintain the code, could it be useful to others.

cfe/trunk/docs/ClangFormatStyleOptions.rst

384 ↗

(On Diff #190379)

Yes that is "as was"

[klimek]

cfe/trunk/docs/ClangFormatStyleOptions.rst

384 ↗

(On Diff #190379)

Ok, that seems... wrong :)
Specifically, I'd at least expect the first one to also do

if (a) return;
else return;

That would be SIS_Always now? If so, could we make "true" just be "Always"?

clang/docs/ClangFormatStyleOptions.rst

* ``SFS_All`` (in configuration: ``All``)
 
     .. code-block:: c++
 
       class Foo {
         void f() { foo(); }
       };
       void f() { bar(); }
 
-
-
 **AllowShortIfStatementsOnASingleLine** (``bool``)
   If ``true``, ``if (a) return;`` can be put on a single line.
 
+  .. code-block:: c++
+
+       if (a) return;
+       else 
+         return;
+
+**AllowShortIfElseStatementsOnASingleLine** (``bool``)
+  When used in conjuction with ``AllowShortIfStatementsOnASingleLine``
+  then when ``true``, ``if (a) return;`` can be put on a single line even when
+  the else clause is a compound statement.
+
+  .. code-block:: c++
+
+       if (a) 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

struct FormatStyle {
 
   /// Dependent on the value, ``int f() { return 0; }`` can be put on a
   /// single line.
   ShortFunctionStyle AllowShortFunctionsOnASingleLine;
 
   /// If ``true``, ``if (a) return;`` can be put on a single line.
   bool AllowShortIfStatementsOnASingleLine;
 
+  /// When used in conjuction with ``AllowShortIfStatementsOnASingleLine``
+  /// then when ``true``, ``if (a) return;`` can be put on a single line even 
+  /// when the else clause is a compound statement.
+  /// \code
+  ///   true:                                   false:
+  ///   if (a) return;                  vs.     if (a)
+  ///   else {                                    return;
+  ///     return;                               else {
+  ///   }                                         return;
+  ///                                           }
+  /// \endcode
+  bool AllowShortIfElseStatementsOnASingleLine;
+
   /// If ``true``, ``while (true) continue;`` can be put on a single
   /// line.
   bool AllowShortLoopsOnASingleLine;
 
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards compatibility.
   enum DefinitionReturnTypeBreakingStyle {
     /// Break after return type automatically.
     /// ``PenaltyReturnTypeOnItsOwnLine`` is taken into account.
     DRTBS_None,
     /// Always break after the return type.
     DRTBS_All,
     /// Always break after the return types of top-level functions.
return AccessModifierOffset == R.AccessModifierOffset &&
                R.AllowAllParametersOfDeclarationOnNextLine &&
            AllowShortBlocksOnASingleLine == R.AllowShortBlocksOnASingleLine &&
            AllowShortCaseLabelsOnASingleLine ==
                R.AllowShortCaseLabelsOnASingleLine &&
            AllowShortFunctionsOnASingleLine ==
                R.AllowShortFunctionsOnASingleLine &&
            AllowShortIfStatementsOnASingleLine ==
                R.AllowShortIfStatementsOnASingleLine &&
+           AllowShortIfElseStatementsOnASingleLine ==
+               R.AllowShortIfElseStatementsOnASingleLine &&
            AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&
            AlwaysBreakAfterReturnType == R.AlwaysBreakAfterReturnType &&
            AlwaysBreakBeforeMultilineStrings ==
                R.AlwaysBreakBeforeMultilineStrings &&
            AlwaysBreakTemplateDeclarations ==
                R.AlwaysBreakTemplateDeclarations &&
            BinPackArguments == R.BinPackArguments &&
            BinPackParameters == R.BinPackParameters &&

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("AllowShortBlocksOnASingleLine",
                    Style.AllowShortBlocksOnASingleLine);
     IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
                    Style.AllowShortCaseLabelsOnASingleLine);
     IO.mapOptional("AllowShortFunctionsOnASingleLine",
                    Style.AllowShortFunctionsOnASingleLine);
     IO.mapOptional("AllowShortIfStatementsOnASingleLine",
                    Style.AllowShortIfStatementsOnASingleLine);
+    IO.mapOptional("AllowShortIfElseStatementsOnASingleLine",
+                   Style.AllowShortIfElseStatementsOnASingleLine);
     IO.mapOptional("AllowShortLoopsOnASingleLine",
                    Style.AllowShortLoopsOnASingleLine);
     IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
                    Style.AlwaysBreakAfterDefinitionReturnType);
     IO.mapOptional("AlwaysBreakAfterReturnType",
                    Style.AlwaysBreakAfterReturnType);
     // If AlwaysBreakAfterDefinitionReturnType was specified but
     // AlwaysBreakAfterReturnType was not, initialize the latter from the
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = false;
   LLVMStyle.AlignConsecutiveDeclarations = false;
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
+  LLVMStyle.AllowShortIfElseStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_MultiLine;
   LLVMStyle.BinPackArguments = true;
   LLVMStyle.BinPackParameters = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;

clang/lib/Format/UnwrappedLineFormatter.cpp

unsigned tryMergeSimpleControlStatement(
     AnnotatedLine &Line = **I;
     if (Line.Last->isNot(tok::r_paren))
       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).
+    // Only inline simple if's (no nested if or else), unless specified
+    if (!Style.AllowShortIfElseStatementsOnASingleLine) {
       if (I + 2 != E && Line.startsWith(tok::kw_if) &&
           I[2]->First->is(tok::kw_else))
         return 0;
+    }
     return 1;
   }
 
   unsigned
   tryMergeShortCaseLabels(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
                           SmallVectorImpl<AnnotatedLine *>::const_iterator E,
                           unsigned Limit) {
     if (Limit == 0 || I + 1 == E ||

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatIfWithoutCompoundStatement) {
   verifyFormat("if (aaaaaaaaa)\n"
                "  return;",
                AllowsMergedIf);
 
   AllowsMergedIf.ColumnLimit = 13;
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  verifyFormat("if (a)\n"
+               "  f();\n"
+               "else {\n"
+               "  g();\n"
+               "}",
+               AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfElseStatementsOnASingleLine = true;
+  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);
+}
+
 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);
   verifyFormat("while (true)\n"
                "  ;",
verifyFormat("for (;;) /* still don't merge */\n"
                AllowsMergedLoops);
 }
 
 TEST_F(FormatTest, FormatShortBracedStatements) {
   FormatStyle AllowSimpleBracedStatements = getLLVMStyle();
   AllowSimpleBracedStatements.ColumnLimit = 40;
   AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
 
   AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
 
   AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
   AllowSimpleBracedStatements.BraceWrapping.AfterFunction = true;
   AllowSimpleBracedStatements.BraceWrapping.SplitEmptyRecord = false;
 
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if constexpr (true) {}", AllowSimpleBracedStatements);