[clang-format] Inconsistent behavior regarding line break before access modifier

[Max_S]

Summary

Fixes https://llvm.org/PR41870.

Checks for newlines in option Style.EmptyLineBeforeAccessModifier are now based on the formatted new lines and not on the new lines in the file.

[Max_S]

This is patch for a bug reported in 2019 https://bugs.llvm.org/show_bug.cgi?id=41870

[MyDeveloperDay]

can you clang-format so it passes the pre-merge checks

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

8977

if you left a line between this test or added your test at the end it wouldn't have cut the existing test in two. I can't easily tell if you are changing or adding tests

[HazardyKnusperkeks]

Look good in general, only the few comments.

clang/unittests/Format/FormatTest.cpp

8892

While we are at empty lines, maybe add some here, so that it is a bit more readable. It took me some time to see that you use 2 different styles.

[Max_S]

Moved the test to their own section and fixed the formatting issues.

[curdeius]

No remarks from me. LGTM.

[Max_S]

Added changelog entry.

[Max_S]

I do not have the access rights to the llvm git repo. Can you please land it for me.

Thanks and thank you for the review.

[HazardyKnusperkeks]

I don't know if you did elsewhere, but you have to give a name and email for the commit, so that someone can push it for you.

[Max_S]

Sorry forgot about that: Max Sagebaum <[e-mail]>

[curdeius]

@Max_S I think you need to rebase on current main and fix two tests as probably D98237 (or another patch) changed the behaviour of this patch.

[  FAILED  ] 2 tests, listed below:
[  FAILED  ] FormatTest.FormatsAccessModifiers
[  FAILED  ] FormatTest.FormatsAfterAccessModifiers

[Max_S]

Sorry for the inconvenience. These were the errors, that inspired this patch.

In all cases the new version is now the correct one. The default setting of EmptyLineBeforeAccessModifier is to insert empty lines for logical blocks, which it does now.

clang/lib/Format/UnwrappedLineFormatter.cpp

if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
       !PreviousLine->startsWithNamespace() &&
       !startsExternCBlock(*PreviousLine))
     Newlines = 1;
 
   // Insert or remove empty line before access specifiers.
   if (PreviousLine && RootToken.isAccessSpecifier()) {
     switch (Style.EmptyLineBeforeAccessModifier) {
     case FormatStyle::ELBAMS_Never:
-      if (RootToken.NewlinesBefore > 1)
+      if (Newlines > 1)
         Newlines = 1;
       break;
     case FormatStyle::ELBAMS_Leave:
       Newlines = std::max(RootToken.NewlinesBefore, 1u);
       break;
     case FormatStyle::ELBAMS_LogicalBlock:
       if (PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
-          RootToken.NewlinesBefore <= 1)
+          Newlines <= 1)
         Newlines = 2;
       break;
     case FormatStyle::ELBAMS_Always: {
       const FormatToken *previousToken;
       if (PreviousLine->Last->is(tok::comment))
         previousToken = PreviousLine->Last->getPreviousNonComment();
       else
         previousToken = PreviousLine->Last;
       if ((!previousToken || !previousToken->is(tok::l_brace)) &&
-          RootToken.NewlinesBefore <= 1)
+          Newlines <= 1)
         Newlines = 2;
     } break;
     }
   }
 
   // Remove empty lines after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BreaksLongDeclarations) {
   verifyFormat("template <typename T> // Templates on own line.\n"
                "static int            // Some comment.\n"
                "MyFunction(int a);",
                getLLVMStyle());
 }
 
 TEST_F(FormatTest, FormatsAccessModifiers) {
   FormatStyle Style = getLLVMStyle();
+  FormatStyle NoEmptyLines = getLLVMStyle();
+  NoEmptyLines.MaxEmptyLinesToKeep = 0;
   EXPECT_EQ(Style.EmptyLineBeforeAccessModifier,
             FormatStyle::ELBAMS_LogicalBlock);
   verifyFormat("struct foo {\n"
                "private:\n"
                "  void f() {}\n"
                "\n"
                "private:\n"
                "  int i;\n"
verifyFormat("struct foo {\n"
                "private:\n"
                "  int i;\n"
                "#ifdef FOO\n"
                "private:\n"
                "#endif\n"
                "  int j;\n"
                "};\n",
                Style);
+  verifyFormat("struct foo {\n"
+               "private:\n"
+               "  void f() {}\n"
+               "\n"
+               "private:\n"
+               "  int i;\n"
+               "\n"
+               "public:\n"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               NoEmptyLines);
   Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  NoEmptyLines.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
   verifyFormat("struct foo {\n"
                "private:\n"
                "  void f() {}\n"
                "private:\n"
                "  int i;\n"
                "protected:\n"
                "  int j;\n"
                "};\n",
                Style);
   verifyFormat("struct foo {\n"
                "private:\n"
                "  void f() {}\n"
                "private:\n"
                "  int i;\n"
+               "public:\n"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               NoEmptyLines);
+  verifyFormat("struct foo {\n"
+               "private:\n"
+               "  void f() {}\n"
+               "private:\n"
+               "  int i;\n"
                "protected:\n"
                "  int j;\n"
                "};\n",
                "struct foo {\n"
                "\n"
                "private:\n"
                "  void f() {}\n"
                "\n"
verifyFormat("struct foo {\n"
                "#ifdef FOO\n"
                "\n"
                "private:\n"
                "#endif\n"
                "  int j;\n"
                "};\n",
                Style);
   Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  NoEmptyLines.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
   verifyFormat("struct foo {\n"
                "private:\n"
                "  void f() {}\n"
                "\n"
                "private:\n"
                "  int i;\n"
                "\n"
                "protected:\n"
                "  int j;\n"
                "};\n",
                Style);
   verifyFormat("struct foo {\n"
                "private:\n"
                "  void f() {}\n"
                "\n"
                "private:\n"
                "  int i;\n"
                "\n"
+               "public:\n"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               NoEmptyLines);
+  verifyFormat("struct foo {\n"
+               "private:\n"
+               "  void f() {}\n"
+               "\n"
+               "private:\n"
+               "  int i;\n"
+               "\n"
                "protected:\n"
                "  int j;\n"
                "};\n",
                "struct foo {\n"
                "private:\n"
                "  void f() {}\n"
                "private:\n"
                "  int i;\n"