[clang-format] Extend AllowShortBlocksOnASingleLine for else blocks

[jessesna]

Summary

Extend AllowShortBlocksOnASingleLine for else blocks. See https://bugs.llvm.org/show_bug.cgi?id=49722

[MyDeveloperDay]

Could you add the case where the else is not empty.

clang/unittests/Format/FormatTest.cpp

5413

Can you please use verifyFormat (as this does some extra stuff to ensure its stable)

[MyDeveloperDay]

Nothing majorly wrong here, its looks pretty good.

[jessesna]

use verifyFormat instead of EXPECT_EQ (as this does some extra stuff to ensure its stable)

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

5419

you can remove the "format(" here now..."

when its 2 arguments to verifyFormat its a kind of verifyFormat(after,before,Style)

alternatively you can use:

verifyFormat(after,Style)

it will deliberately messUp the the text and ensure it resets itself back to the desired after

[jessesna]

use the more dense "verifyFormat(after,Style)" calls

[MyDeveloperDay]

I agree I think this LGTM

[owenpan]

Should we add an if-else example to the documentation?

[MyDeveloperDay]

Ok now I'm a little puzzled, shouldn't this be covered by AllowShortIfStatementsOnASingleLine?

I think at a minimum we need to understand why those options don't work, and I agree we should add a ReleaseNote and add something to the documentation (by editing Format.h)

[jessesna]

In D114320#3148046, @MyDeveloperDay wrote:

Ok now I'm a little puzzled, shouldn't this be covered by AllowShortIfStatementsOnASingleLine?

That's exactly what i thought when i first stumbled over the behavior :D But I think the majority of users might not even consider adding empty else blocks. Only a few either just do it or have to comply with some standards, f.e. MISRA .

[MyDeveloperDay]

In D114320#3151741, @jessesna wrote:

In D114320#3148046, @MyDeveloperDay wrote:

Ok now I'm a little puzzled, shouldn't this be covered by AllowShortIfStatementsOnASingleLine?

That's exactly what i thought when i first stumbled over the behavior :D But I think the majority of users might not even consider adding empty else blocks. Only a few either just do it or have to comply with some standards, f.e. MISRA .

I tend to agree with you.

[MyDeveloperDay]

Thank you for the patch, Do you need help committing this? if so we need your name and email address to do so

https://llvm.org/docs/DeveloperPolicy.html#commit-messages

[jessesna]

In D114320#3153795, @MyDeveloperDay wrote:

Thank you for the patch, Do you need help committing this? if so we need your name and email address to do so

https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Thanks for the reminder. I wasn't sure about where we are in the process. I just did a rebase, compiled and run the tests again and it's all still fine but this is my first contribution and i don't think i have commit access according to https://www.llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access . I'd be glad if someone else could commit for me. What's the best method to send the email address?

[MyDeveloperDay]

one of us can commit it for you but we need to do the following to attribute it to you

commit --amend --author="John Doe <[e-mail]>"

So I need your name and email address.

[jessesna]

Great. Thanks a lot. It's Jesses Gott and [e-mail]

[jessesna]

Thanks a lot for your time and help @MyDeveloperDay

[MyDeveloperDay]

In D114320#3154544, @jessesna wrote:

Thanks a lot for your time and help @MyDeveloperDay

No problem, come play some more! We need people to help and review.

clang/lib/Format/UnwrappedLineFormatter.cpp

tryFitMultipleLinesInOne(LevelIndentTracker &IndentTracker,
     if (TheLine->Last->is(tok::l_brace) && TheLine->First != TheLine->Last &&
         TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
       return Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;
     }
     // Try to merge a control statement block with left brace wrapped
     if (I[1]->First->is(tok::l_brace) &&
-        (TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
-                                 tok::kw_switch, tok::kw_try, tok::kw_do,
-                                 TT_ForEachMacro) ||
+        (TheLine->First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while,
+                                 tok::kw_for, tok::kw_switch, tok::kw_try,
+                                 tok::kw_do, TT_ForEachMacro) ||
          (TheLine->First->is(tok::r_brace) && TheLine->First->Next &&
           TheLine->First->Next->isOneOf(tok::kw_else, tok::kw_catch))) &&
         Style.BraceWrapping.AfterControlStatement ==
             FormatStyle::BWACS_MultiLine) {
       // If possible, merge the next line's wrapped left brace with the current
       // line. Otherwise, leave it on the next line, as this is a multi-line
       // control statement.
       return (Style.ColumnLimit == 0 ||
               TheLine->Last->TotalLength <= Style.ColumnLimit)
                  ? 1
                  : 0;
     } else if (I[1]->First->is(tok::l_brace) &&
-               TheLine->First->isOneOf(tok::kw_if, tok::kw_while,
+               TheLine->First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while,
                                        tok::kw_for)) {
       return (Style.BraceWrapping.AfterControlStatement ==
               FormatStyle::BWACS_Always)
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;
     } else if (I[1]->First->is(tok::l_brace) &&
                TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
                Style.BraceWrapping.AfterControlStatement ==
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 == FormatStyle::SBS_Never)
         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) &&
+          Line.First->isOneOf(tok::kw_if, tok::kw_else) &&
           !Style.BraceWrapping.AfterControlStatement &&
           !I[1]->First->is(tok::r_brace))
         return 0;
       if (!Style.AllowShortIfStatementsOnASingleLine &&
-          Line.startsWith(tok::kw_if) &&
+          Line.First->isOneOf(tok::kw_if, tok::kw_else) &&
           Style.BraceWrapping.AfterControlStatement ==
               FormatStyle::BWACS_Always &&
           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

TEST_F(FormatTest, PutEmptyBlocksIntoOneLine) {
   EXPECT_EQ("{}", format("{}"));
   verifyFormat("enum E {};");
   verifyFormat("enum E {}");
   FormatStyle Style = getLLVMStyle();
   Style.SpaceInEmptyBlock = true;
   EXPECT_EQ("void f() { }", format("void f() {}", Style));
   Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Empty;
   EXPECT_EQ("while (true) { }", format("while (true) {}", Style));
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.BeforeElse = false;
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
+  verifyFormat("if (a)\n"
+               "{\n"
+               "} else if (b)\n"
+               "{\n"
+               "} else\n"
+               "{ }",
+               Style);
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
+  verifyFormat("if (a) {\n"
+               "} else if (b) {\n"
+               "} else {\n"
+               "}",
+               Style);
+  Style.BraceWrapping.BeforeElse = true;
+  verifyFormat("if (a) { }\n"
+               "else if (b) { }\n"
+               "else { }",
+               Style);
 }
 
 TEST_F(FormatTest, FormatBeginBlockEndMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.MacroBlockBegin = "^[A-Z_]+_BEGIN$";
   Style.MacroBlockEnd = "^[A-Z_]+_END$";
   verifyFormat("FOO_BEGIN\n"
                "  FOO_ENTRY\n"