[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

8962

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/docs/ReleaseNotes.rst

- Option ``ShortNamespaceLines`` has been added to give better control
   over ``FixNamespaceComments`` when determining a namespace length.
 
 - Support for Whitesmiths has been improved, with fixes for ``namespace`` blocks
   and ``case`` blocks and labels.
 
 - Option ``EmptyLineAfterAccessModifier`` has been added to remove, force or keep
   new lines after access modifiers.
 
+- Checks for newlines in option ``EmptyLineBeforeAccessModifier`` are now based
+  on the formatted new lines and not on the new lines in the file. (Fixes
+  https://llvm.org/PR41870.)
+
 libclang
 --------
 
 - ...
 
 Static Analyzer
 ---------------
 

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)
+      if (PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) && Newlines <= 1)
         Newlines = 2;
       if (PreviousLine->First->isAccessSpecifier())
         Newlines = 1; // Previous is an access modifier remove all new lines.
       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)
+      if ((!previousToken || !previousToken->is(tok::l_brace)) && Newlines <= 1)
         Newlines = 2;
     } break;
     }
   }
 
   // Insert or remove empty line 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();
   EXPECT_EQ(Style.EmptyLineBeforeAccessModifier,
             FormatStyle::ELBAMS_LogicalBlock);
   verifyFormat("struct foo {\n"
                "private:\n"
                "  void f() {}\n"
                "\n"
                "private:\n"
                "  int i;\n"
                "\n"
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"
                "private:\n"
                "  int i;\n"
                "\n"
                "protected:\n"
                "  int j;\n"
verifyFormat("struct foo {\n"
                "private:\n"
                "  int i;\n"
                "#ifdef FOO\n"
                "private:\n"
                "#endif\n"
                "  int j;\n"
                "};\n",
                Style);
+
+  FormatStyle NoEmptyLines = getLLVMStyle();
+  NoEmptyLines.MaxEmptyLinesToKeep = 0;
+  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);
+
+  NoEmptyLines.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+               "private:\n"
+               "  void f() {}\n"
+               "private:\n"
+               "  int i;\n"
+               "public:\n"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               NoEmptyLines);
+
+  NoEmptyLines.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"
+               "private:\n"
+               "  void f() {}\n"
+               "\n"
+               "private:\n"
+               "  int i;\n"
+               "\n"
+               "public:\n"
+               "\n"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               NoEmptyLines);
 }
 
 TEST_F(FormatTest, FormatsAfterAccessModifiers) {
 
   FormatStyle Style = getLLVMStyle();
   EXPECT_EQ(Style.EmptyLineAfterAccessModifier, FormatStyle::ELAAMS_Never);
   verifyFormat("struct foo {\n"
                "private:\n"
verifyFormat("struct foo {\n"
                "protected:\n"
                "  int j;\n"
                "};\n",
                Style);
 
   // Leave tests rely on the code layout, test::messUp can not be used.
   Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
   Style.MaxEmptyLinesToKeep = 0u;
-  EXPECT_EQ("struct foo {\n"
-            "private:\n"
-            "  void f() {}\n"
-            "private:\n"
-            "  int i;\n"
-            "protected:\n"
-            "  int j;\n"
-            "};\n",
-            format("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"
+               Style);
 
   // Check if MaxEmptyLinesToKeep is respected.
   EXPECT_EQ("struct foo {\n"
             "private:\n"
             "  void f() {}\n"
+            "\n"
             "private:\n"
             "  int i;\n"
+            "\n"
             "protected:\n"
             "  int j;\n"
             "};\n",
             format("struct foo {\n"
                    "private:\n"
                    "\n\n\n"
                    "  void f() {}\n"
                    "\n"