[clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[thezbyg]

Summary

Add new option called InsertEmptyLineBeforeAccessModifier. Empty line before access modifier is inserted if this option is set to true (which is the default value, because clang-format always inserts empty lines before access modifiers), otherwise empty lines are removed.

Fixes issue #16518.

[MyDeveloperDay]

Thank you for this patch, a few process issues.

clang/include/clang/Format/Format.h

58

you need to run clang/doc/tools/dump_style.py so that it regenerates the ClangFormatsStyleOptions.rst file

68

quite a mouthful... maybe just NewLineBeforeAccessModifier ?

clang/test/Format/access-modifiers.cpp

2

This is good to add, but could you also add a test in clang/unittests/Format/FormatTests.cpp

[curdeius]

My 2 cents. Otherwise I agree with @MyDeveloperDay's comments.

clang/lib/Format/UnwrappedLineFormatter.cpp

1218–1247

Given that the two ifs handle the same construct, I think we should try to merge them to avoid duplication in conditions. Sth like:

if (PreviousLine && RootToken.isAccessSpecifier()) {
    if (... Style.Insert ... && ... ) {
    ...
    }
    else if ( (... !Style.Insert ... && ... ) {
    ...
    }
}

1219

Just thinking out loud, but shouldn't we just check that PreviousLine->Last->isNot(tok::l_brace)?
Could you please add a test with comments before access modifiers?

1248–1249

Please add tests with preprocessor directives *before* access modifiers too.

1249

Is it really needed to check the type of PreviousLine->Last?

[HazardyKnusperkeks]

clang/include/clang/Format/Format.h

68

A new line is always there, how about EmptyLineBeforeAccessModifier?

Apart from that, I would prefer an alphabetical sorting of the member, most of them are (not all I know...).

2862

Here it is sorted.

clang/lib/Format/Format.cpp

457

Here again not.

[MyDeveloperDay]

clang/include/clang/Format/Format.h

68

Yes EmptyLine seems the better termininology

[thezbyg]

Option renamed to EmptyLineBeforeAccessModifier.
Placed new configuration member in correct place alphabetically.
Last token in previous line is no longer checked when EmptyLineBeforeAccessModifier is false.
Executed clang/doc/tools/dump_style.py to update ClangFormatsStyleOptions.rst.
Unit tests added to clang/unittests/Format/FormatTests.cpp.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

8891–8903

if you use verifyFormat it will check what happens when it messes the code up to ensure its stable

[thezbyg]

clang/lib/Format/UnwrappedLineFormatter.cpp

1219

No previous line token check is necessary if it is acceptable to remove empty lines in situations where EmptyLineBeforeAccessModifier=true would not add empty line.

Without any additional tests formatting the following code with EmptyLineBeforeAccessModifier=true:

struct foo {
private:
  int i;
}

would not add any empty lines, but formatting this code with EmptyLineBeforeAccessModifier=false:

struct foo {

private:
  int i;
}

would remove empty line before modifier.

[thezbyg]

clang/unittests/Format/FormatTest.cpp

8891–8903

After switching to verifyFormat all tests pass when checking C++ formatting, but some of the same tests fail in Objective-C++ check:
C style comment is attached to previous line:

      Expected: Expected.str()
      Which is: "struct foo {\n  /* comment */\nprivate:\n  int i;\n  int j;\n}\n"
To be equal to: format(test::messUp(Code), ObjCStyle)
      Which is: "struct foo { /* comment */\nprivate:\n  int i;\n  int j;\n}\n"
With diff:
@@ -1,4 +1,3 @@
-struct foo {
-  /* comment */
+struct foo { /* comment */
 private:
   int i;

Empty line before access modifier is removed:

      Expected: Expected.str()
      Which is: "struct foo {\n  int i;\n\nprivate:\n  int j;\n}\n"
To be equal to: format(test::messUp(Code), ObjCStyle)
      Which is: "struct foo {\n  int i;\nprivate:\n  int j;\n}\n"
With diff:
@@ -1,5 @@
 struct foo {
   int i;
-
 private:
   int j;

Looks like empty lines before modifiers are removed for Objective-C++ language. What should I do?

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

8891–8903

please update the patch at least to user verifyFomat where you can.

Its likely means we are missing something

[thezbyg]

Switched to verifyFormat in most of the tests.
Rerun clang-format.

[curdeius]

Nit.

clang/docs/ClangFormatStyleOptions.rst

1877

Full stop at the end of the phrase.

[MyDeveloperDay]

clang/include/clang/Format/Format.h

1644

The full stop will go here then regenerate.

[thezbyg]

Added missing full stop.
Executed clang/doc/tools/dump_style.py to update ClangFormatStyleOptions.rst.

[MyDeveloperDay]

ok so this looks ok, but before we commit can we have a discussion about why you think it fails for the comment case?

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

8891

any reason this can't be verifyFormat?

8912

verifyFormat? why not

[thezbyg]

After some updating, rebuilding and searching for differences in Objective-C++ formatting, I managed to find where the problem with these failing tests is. In _verifyFormat function C++ code formatting is tested for stability like this:

EXPECT_EQ(Expected.str(), format(Expected, Style));
EXPECT_EQ(Expected.str(), format(Code, Style));

, but Objective-C++ test, in the same function, looks like this:

EXPECT_EQ(Expected.str(), format(test::messUp(Code), ObjCStyle));

test::messUp function removes all newlines and indentation, so test code:

struct foo {
  int i;

private:
  int j;
}

turns into:

struct foo { int i; private: int j; }

After running format on this code, we get incorrect result:

struct foo {
  int i;
private:
  int j;
}

Running format again would produce the correct output:

struct foo {
  int i;

private:
  int j;
}

So it seems that insertion of empty line fails when access modifier is in the same line as previous tokens. Unmodified clang-format produces the same output. As this behavior is not related to the changes in my patch, should we attempt to fix it here, or a separate bug report would be preferred?

The situation with tests containing comments is similar, because test::messUp single line output is formatted into:

struct foo { /* comment */
private:
  int i;
  int j;
}

which is not the same as:

struct foo {
  /* comment */
private:
  int i;
  int j;
}

In my opinion, Objective-C++ test::messUp test incorrectly expects any code collapsed into a single line to format into the same code as formatted original code.

[thezbyg]

When access modifier is in the same line with previous tokens, UnwrappedLineFormatter::formatFirstToken is called with RootToken.NewlinesBefore == 0, but empty line is only inserted if RootToken.NewlinesBefore == 1. The following change fixes this and passes all tests except the ones with comment:

   if (PreviousLine && RootToken.isAccessSpecifier()) {
     if (Style.EmptyLineBeforeAccessModifier &&
         PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
-        RootToken.NewlinesBefore == 1)
-      ++Newlines;
+        RootToken.NewlinesBefore <= 1)
+      Newlines = 2;
     else if (!Style.EmptyLineBeforeAccessModifier &&
              RootToken.NewlinesBefore > 1)
       Newlines = 1;
   }

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineFormatter.cpp

1222

maybe I don't understand the logic but why is this r_brace and shouldn't we be looking at the "Last Non Comment" token?

[thezbyg]

clang/lib/Format/UnwrappedLineFormatter.cpp

1222

I think that the logic is to only add empty line when access modifier is after member function body (r_brace indicates this) or declaration of field/function. If this check is changed to look at "last non comment" token, then empty line will be inserted in code like this:

struct Foo {
  int i;
  //comment
private:
  int j;
}

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

1222

But with the given Name it should add an empty line there! Otherwise you should use an enum and not a bool to control wether a comment should suppress the empty line or not. Also you should make the exception(s) clear with unit tests.

1223–1224

A tab?

clang/unittests/Format/FormatTest.cpp

8959–8972

Just curios, any reason why the struct is repeated? I don't seem to notice a difference.

And by the way, you are missing the ; at the end of the definition, I don't know if that affects clang-format.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineFormatter.cpp

1223–1224

My experience is that this doesn't mean a tab but just the what phabricator shows a change in whitespace

it is now 2 levels of if scope indented not 1 so I think it should be moved over abit

[thezbyg]

clang/lib/Format/UnwrappedLineFormatter.cpp

1222

Current clang-format behavior of access modifier separation was added in commit (fd433365e08da024ddc99dd0d7bce8e89d8c5469) and the test function is called SeparatesLogicalBlocks. So could we simply rename this new option to SeparateLogicalBlocks and leave the bool type? Option description would contain code examples and further explanation what logical block means. Setting SeparateLogicalBlocks option to false would disable empty line insertion, but would not remove existing empty lines.

Is option name EmptyLineBeforeAccessModifier acceptable if enum is used? And how should I name enum values? One enum value could be called Never, but other must indicate that empty line is only inserted when access modifier is after member function body or field/function/struct declaration.

I think that you incorrectly assumed from my previous comment, that only comments suppress empty line insertion. No empty line is inserted in all following code examples:

struct Foo {
private:
  int j;
};

struct Foo {
private:
public:
  int j;
};

struct Foo {
#ifdef FOO
private:
#endif
  int j;
};

1223–1224

Yes, there is no tab in submitted patch, only 6 spaces.

Is this indented incorrectly?

if (PreviousLine && RootToken.isAccessSpecifier()) {
  if (Style.EmptyLineBeforeAccessModifier &&
      PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
      RootToken.NewlinesBefore == 1)
    ++Newlines;
  else if (!Style.EmptyLineBeforeAccessModifier &&
           RootToken.NewlinesBefore > 1)
    Newlines = 1;
}

I always run git clang-format-11 HEAD~1 before generating patch file and uploading it here.

clang/unittests/Format/FormatTest.cpp

8959–8972

Some of the tests have equal expected and code values. I will update them to single parameter verifyFormat().

clang-format does not seem to care about missing ;, but I will add them as all other tests have them.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

1222

Current clang-format behavior of access modifier separation was added in commit (fd433365e08da024ddc99dd0d7bce8e89d8c5469) and the test function is called SeparatesLogicalBlocks. So could we simply rename this new option to SeparateLogicalBlocks and leave the bool type? Option description would contain code examples and further explanation what logical block means. Setting SeparateLogicalBlocks option to false would disable empty line insertion, but would not remove existing empty lines.

Is option name EmptyLineBeforeAccessModifier acceptable if enum is used? And how should I name enum values? One enum value could be called Never, but other must indicate that empty line is only inserted when access modifier is after member function body or field/function/struct declaration.

That now depends on what you want (at least for me), if the name stays EmptyLineBeforeAccessModifier and it stays a bool it should nearly always add a space (most likely not directly after the {. I think the name would still fit for an enum with values like Always, Never, DontModify, and similar.

If you change the name it can stay a bool, but then needs an explanation with examples what the option really means.

1223–1224

I would assume so. I was just irritated by the >>. Everything is fine here.

[thezbyg]

Switched EmptyLineBeforeAccessModifierStyle option from bool to enum.
EmptyLineBeforeAccessModifierStyle option can now have one of four values: Never, DontModify, LogicalBlock, Always.
Never removes all empty lines before access modifiers.
DontModify keeps existing empty lines.
LogicalBlock adds empty line only when access modifier starts a new logical block (current clang-format behavior).
Always adds empty line before all access modifiers except when access modifier is at the start of class/struct definition.

Updated tests to test all four option values. When testing DontModify option, some of the tests use EXPECT_EQ() instead of verifyFormat(). This is because empty lines are not preserved in verifyFormat() due to test::messUp().

[HazardyKnusperkeks]

I think your base for the diff is wrong, there are many "added" things which are already in clang-format. Could you update the diff?

[thezbyg]

Diff updated. Previous diff was generated after rebase, and Phabricator change preview did not show any unrelated changes, so I thought that everything is fine.

[HazardyKnusperkeks]

In D93846#2488469, @thezbyg wrote:

Diff updated. Previous diff was generated after rebase, and Phabricator change preview did not show any unrelated changes, so I thought that everything is fine.

Now your diff removes the stuff?
You should rebase your change on master or main, (ideally) have only one commit on top of it, and than diff HEAD^1.

[thezbyg]

Rebased changes on master.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

235

Other places use Leave I assume that means the same as DontModify?

[HazardyKnusperkeks]

Apart from that last naming issue looks good to me.

clang/lib/Format/Format.cpp

235

If that's the case, we should stick with it. Although I personally find DontModify better.

[thezbyg]

Renamed DontModify enum to Leave.

[curdeius]

One last nit, otherwise LGTM.

clang/unittests/Format/FormatTest.cpp

8891

For the ease of understanding that you test LogicalBlock in this first part, I'd add:

FormatStyle Style = getLLVMStyle();
EXPECT_EQ(Style.EmptyLineBeforeAccessModifier, FormatStyle::ELBAMS_LogicalBlock);

and use this Style in verifyFormat.

[thezbyg]

Improved default style tests.

[HazardyKnusperkeks]

Do you need someone to push this? If so please post email and name to use for the commit.

[curdeius]

clang/unittests/Format/FormatTest.cpp

8943

Redundant line.

[thezbyg]

Remove redundant line.

[thezbyg]

Yes, I do not have commit access and would like for someone to commit/push this change. My name and email is Albertas Vyšniauskas <[e-mail]>.

clang/docs/ClangFormatStyleOptions.rst

**DerivePointerAlignment** (``bool``)
   alignment of ``&`` and ``*``.
   Pointer and reference alignment styles are going to be updated according
   to the preferences found in the file.
   ``PointerAlignment`` is then used only as fallback.
 
 **DisableFormat** (``bool``)
   Disables formatting completely.
 
+**EmptyLineBeforeAccessModifier** (``EmptyLineBeforeAccessModifierStyle``)
+  Defines in which cases to put empty line before access modifiers.
+
+  Possible values:
+
+  * ``ELBAMS_Never`` (in configuration: ``Never``)
+    Remove all empty lines before access modifiers.
+
+    .. code-block:: c++
+
+      struct foo {
+      private:
+        int i;
+      protected:
+        int j;
+        /* comment */
+      public:
+        foo() {}
+      private:
+      protected:
+      };
+
+  * ``ELBAMS_Leave`` (in configuration: ``Leave``)
+    Keep existing empty lines before access modifiers.
+
+  * ``ELBAMS_LogicalBlock`` (in configuration: ``LogicalBlock``)
+    Add empty line only when access modifier starts a new logical block.
+    Logical block is a group of one or more member fields or functions.
+
+    .. code-block:: c++
+
+      struct foo {
+      private:
+        int i;
+
+      protected:
+        int j;
+        /* comment */
+      public:
+        foo() {}
+
+      private:
+      protected:
+      };
+
+  * ``ELBAMS_Always`` (in configuration: ``Always``)
+    Always add empty line before access modifiers unless access modifier
+    is at the start of struct or class definition.
+
+    .. code-block:: c++
+
+      struct foo {
+      private:
+        int i;
+
+      protected:
+        int j;
+        /* comment */
+
+      public:
+        foo() {}
+
+      private:
+
+      protected:
+      };
+
+
+
 **ExperimentalAutoDetectBinPacking** (``bool``)
   If ``true``, clang-format detects whether function calls and
   definitions are formatted with one parameter per line.
 
   Each call can be bin-packed, one-per-line or inconclusive. If it is
   inconclusive, e.g. completely on one line, but a decision needs to be
   made, clang-format analyzes whether there are other bin-packed cases in
   the input file and act accordingly.

clang/include/clang/Format/Format.h

 std::error_code make_error_code(ParseError e);
 
 /// The ``FormatStyle`` is used to configure the formatting to follow
 /// specific guidelines.
 struct FormatStyle {
   /// The extra indent or outdent of access modifiers, e.g. ``public:``.
   int AccessModifierOffset;
 
   /// Different styles for aligning after open brackets.
   enum BracketAlignmentStyle : unsigned char {
     /// Align parameters on the open bracket, e.g.:
     /// \code
     ///   someLongFunction(argument1,
     ///                    argument2);
     /// \endcode
     BAS_Align,
     /// Don't align, instead use ``ContinuationIndentWidth``, e.g.:
     /// \code
     ///   someLongFunction(argument1,
     ///       argument2);
     /// \endcode
     BAS_DontAlign,
     /// Always break after an open bracket, if the parameters don't fit
     /// on a single line, e.g.:
     /// \code
     ///   someLongFunction(
     ///       argument1, argument2);

   /// Pointer and reference alignment styles are going to be updated according
   /// to the preferences found in the file.
   /// ``PointerAlignment`` is then used only as fallback.
   bool DerivePointerAlignment;
 
   /// Disables formatting completely.
   bool DisableFormat;
 
+  /// Different styles for empty line before access modifiers.
+  enum EmptyLineBeforeAccessModifierStyle : unsigned char {
+    /// Remove all empty lines before access modifiers.
+    /// \code
+    ///   struct foo {
+    ///   private:
+    ///     int i;
+    ///   protected:
+    ///     int j;
+    ///     /* comment */
+    ///   public:
+    ///     foo() {}
+    ///   private:
+    ///   protected:
+    ///   };
+    /// \endcode
+    ELBAMS_Never,
+    /// Keep existing empty lines before access modifiers.
+    ELBAMS_Leave,
+    /// Add empty line only when access modifier starts a new logical block.
+    /// Logical block is a group of one or more member fields or functions.
+    /// \code
+    ///   struct foo {
+    ///   private:
+    ///     int i;
+    ///
+    ///   protected:
+    ///     int j;
+    ///     /* comment */
+    ///   public:
+    ///     foo() {}
+    ///
+    ///   private:
+    ///   protected:
+    ///   };
+    /// \endcode
+    ELBAMS_LogicalBlock,
+    /// Always add empty line before access modifiers unless access modifier
+    /// is at the start of struct or class definition.
+    /// \code
+    ///   struct foo {
+    ///   private:
+    ///     int i;
+    ///
+    ///   protected:
+    ///     int j;
+    ///     /* comment */
+    ///
+    ///   public:
+    ///     foo() {}
+    ///
+    ///   private:
+    ///
+    ///   protected:
+    ///   };
+    /// \endcode
+    ELBAMS_Always,
+  };
+
+  /// Defines in which cases to put empty line before access modifiers.
+  EmptyLineBeforeAccessModifierStyle EmptyLineBeforeAccessModifier;
+
   /// If ``true``, clang-format detects whether function calls and
   /// definitions are formatted with one parameter per line.
   ///
   /// Each call can be bin-packed, one-per-line or inconclusive. If it is
   /// inconclusive, e.g. completely on one line, but a decision needs to be
   /// made, clang-format analyzes whether there are other bin-packed cases in
   /// the input file and act accordingly.
   ///
return AccessModifierOffset == R.AccessModifierOffset &&
                R.ConstructorInitializerAllOnOneLineOrOnePerLine &&
            ConstructorInitializerIndentWidth ==
                R.ConstructorInitializerIndentWidth &&
            ContinuationIndentWidth == R.ContinuationIndentWidth &&
            Cpp11BracedListStyle == R.Cpp11BracedListStyle &&
            DeriveLineEnding == R.DeriveLineEnding &&
            DerivePointerAlignment == R.DerivePointerAlignment &&
            DisableFormat == R.DisableFormat &&
+           EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier &&
            ExperimentalAutoDetectBinPacking ==
                R.ExperimentalAutoDetectBinPacking &&
            FixNamespaceComments == R.FixNamespaceComments &&
            ForEachMacros == R.ForEachMacros &&
            IncludeStyle.IncludeBlocks == R.IncludeStyle.IncludeBlocks &&
            IncludeStyle.IncludeCategories == R.IncludeStyle.IncludeCategories &&
            IncludeStyle.IncludeIsMainRegex ==
                R.IncludeStyle.IncludeIsMainRegex &&
            IncludeStyle.IncludeIsMainSourceRegex ==
                R.IncludeStyle.IncludeIsMainSourceRegex &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentCaseBlocks == R.IndentCaseBlocks &&
            IndentGotoLabels == R.IndentGotoLabels &&
            IndentPragmas == R.IndentPragmas &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentExternBlock == R.IndentExternBlock &&
            IndentRequires == R.IndentRequires && IndentWidth == R.IndentWidth &&
            Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
            JavaImportGroups == R.JavaImportGroups &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&
            KeepEmptyLinesAtTheStartOfBlocks ==
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
            NamespaceIndentation == R.NamespaceIndentation &&

clang/lib/Format/Format.cpp

static void enumeration(IO &IO,
                           FormatStyle::BreakInheritanceListStyle &Value) {
     IO.enumCase(Value, "BeforeColon", FormatStyle::BILS_BeforeColon);
     IO.enumCase(Value, "BeforeComma", FormatStyle::BILS_BeforeComma);
     IO.enumCase(Value, "AfterColon", FormatStyle::BILS_AfterColon);
   }
 };
 
 template <>
+struct ScalarEnumerationTraits<
+    FormatStyle::EmptyLineBeforeAccessModifierStyle> {
+  static void
+  enumeration(IO &IO, FormatStyle::EmptyLineBeforeAccessModifierStyle &Value) {
+    IO.enumCase(Value, "Never", FormatStyle::ELBAMS_Never);
+    IO.enumCase(Value, "Leave", FormatStyle::ELBAMS_Leave);
+    IO.enumCase(Value, "LogicalBlock", FormatStyle::ELBAMS_LogicalBlock);
+    IO.enumCase(Value, "Always", FormatStyle::ELBAMS_Always);
+  }
+};
+
+template <>
 struct ScalarEnumerationTraits<FormatStyle::PPDirectiveIndentStyle> {
   static void enumeration(IO &IO, FormatStyle::PPDirectiveIndentStyle &Value) {
     IO.enumCase(Value, "None", FormatStyle::PPDIS_None);
     IO.enumCase(Value, "AfterHash", FormatStyle::PPDIS_AfterHash);
     IO.enumCase(Value, "BeforeHash", FormatStyle::PPDIS_BeforeHash);
   }
 };
 
if (!IO.outputting()) {
       IO.mapOptional("IndentFunctionDeclarationAfterType",
                      Style.IndentWrappedFunctionNames);
       IO.mapOptional("PointerBindsToType", Style.PointerAlignment);
       IO.mapOptional("SpaceAfterControlStatementKeyword",
                      Style.SpaceBeforeParens);
     }
 
     IO.mapOptional("AccessModifierOffset", Style.AccessModifierOffset);
     IO.mapOptional("AlignAfterOpenBracket", Style.AlignAfterOpenBracket);
     IO.mapOptional("AlignConsecutiveMacros", Style.AlignConsecutiveMacros);
     IO.mapOptional("AlignConsecutiveAssignments",
                    Style.AlignConsecutiveAssignments);
     IO.mapOptional("AlignConsecutiveBitFields",
                    Style.AlignConsecutiveBitFields);
     IO.mapOptional("AlignConsecutiveDeclarations",
                    Style.AlignConsecutiveDeclarations);
     IO.mapOptional("AlignEscapedNewlines", Style.AlignEscapedNewlines);
IO.mapOptional("ConstructorInitializerAllOnOneLineOrOnePerLine",
                    Style.ConstructorInitializerAllOnOneLineOrOnePerLine);
     IO.mapOptional("ConstructorInitializerIndentWidth",
                    Style.ConstructorInitializerIndentWidth);
     IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
     IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
     IO.mapOptional("DeriveLineEnding", Style.DeriveLineEnding);
     IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
     IO.mapOptional("DisableFormat", Style.DisableFormat);
+    IO.mapOptional("EmptyLineBeforeAccessModifier",
+                   Style.EmptyLineBeforeAccessModifier);
     IO.mapOptional("ExperimentalAutoDetectBinPacking",
                    Style.ExperimentalAutoDetectBinPacking);
     IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
     IO.mapOptional("ForEachMacros", Style.ForEachMacros);
     IO.mapOptional("IncludeBlocks", Style.IncludeStyle.IncludeBlocks);
     IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
     IO.mapOptional("IncludeIsMainSourceRegex",
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";
   LLVMStyle.CompactNamespaces = false;
   LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   LLVMStyle.ConstructorInitializerIndentWidth = 4;
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DeriveLineEnding = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
   LLVMStyle.ExperimentalAutoDetectBinPacking = false;
   LLVMStyle.FixNamespaceComments = true;
   LLVMStyle.ForEachMacros.push_back("foreach");
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IncludeStyle.IncludeCategories = {
       {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
       {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},

clang/lib/Format/UnwrappedLineFormatter.cpp

if (RootToken.IsFirst && !RootToken.HasUnescapedNewline)
     Newlines = 0;
 
   // Remove empty lines after "{".
   if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
       PreviousLine->Last->is(tok::l_brace) &&
       !PreviousLine->startsWithNamespace() &&
       !startsExternCBlock(*PreviousLine))
     Newlines = 1;
 
-  // Insert extra new line before access specifiers.
-  if (PreviousLine && PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
-      RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)
-    ++Newlines;
+  // Insert or remove empty line before access specifiers.
+  if (PreviousLine && RootToken.isAccessSpecifier()) {
+    switch (Style.EmptyLineBeforeAccessModifier) {
+    case FormatStyle::ELBAMS_Never:
+      if (RootToken.NewlinesBefore > 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 = 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 = 2;
+    } break;
+    }
+  }
 
   // Remove empty lines after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
     Newlines = std::min(1u, Newlines);
 
   if (Newlines)
     Indent = NewlineIndent;
 
   // If in Whitemsmiths mode, indent start and end of blocks
   if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
     if (RootToken.isOneOf(tok::l_brace, tok::r_brace, tok::kw_case,

clang/test/Format/access-modifiers.cpp

+// RUN: grep -Ev "// *[A-Z-]+:" %s \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, EmptyLineBeforeAccessModifier: LogicalBlock}" -lines=1:14 \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, EmptyLineBeforeAccessModifier: Never}" -lines=14:40 \
+// RUN:   | FileCheck -strict-whitespace %s
+
+// CHECK: int i
+// CHECK-NEXT: {{^$}}
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct foo1 {
+  int i;
+
+private:
+  int j;
+};
+
+// CHECK: struct bar1
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct bar1 {
+private:
+  int i;
+  int j;
+};
+
+// CHECK: int i
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct foo2 {
+  int i;
+
+private:
+  int j;
+};
+
+// CHECK: struct bar2
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct bar2 {
+private:
+  int i;
+  int j;
+};
+
+// CHECK: int j
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct foo3 {
+  int i;
+  int j;
+
+private:
+};
+
+// CHECK: struct bar3
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct bar3 {
+
+private:
+  int i;
+  int j;
+};

clang/unittests/Format/FormatTest.cpp

verifyFormat("void aaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
 
   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"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  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"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  verifyFormat("struct foo { /* comment */\n"
+               "private:\n"
+               "  int i;\n"
+               "  // comment\n"
+               "private:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO\n"
+               "#endif\n"
+               "private:\n"
+               "  int i;\n"
+               "#ifdef FOO\n"
+               "private:\n"
+               "#endif\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  Style.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"
+               "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"
+               "};\n",
+               Style);
+  verifyFormat("struct foo { /* comment */\n"
+               "private:\n"
+               "  int i;\n"
+               "  // comment\n"
+               "private:\n"
+               "  int j;\n"
+               "};\n",
+               "struct foo { /* comment */\n"
+               "\n"
+               "private:\n"
+               "  int i;\n"
+               "  // comment\n"
+               "\n"
+               "private:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO\n"
+               "#endif\n"
+               "private:\n"
+               "  int i;\n"
+               "#ifdef FOO\n"
+               "private:\n"
+               "#endif\n"
+               "  int j;\n"
+               "};\n",
+               "struct foo {\n"
+               "#ifdef FOO\n"
+               "#endif\n"
+               "\n"
+               "private:\n"
+               "  int i;\n"
+               "#ifdef FOO\n"
+               "\n"
+               "private:\n"
+               "#endif\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  Style.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"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               "struct foo {\n"
+               "private:\n"
+               "  void f() {}\n"
+               "private:\n"
+               "  int i;\n"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  verifyFormat("struct foo { /* comment */\n"
+               "private:\n"
+               "  int i;\n"
+               "  // comment\n"
+               "\n"
+               "private:\n"
+               "  int j;\n"
+               "};\n",
+               "struct foo { /* comment */\n"
+               "private:\n"
+               "  int i;\n"
+               "  // comment\n"
+               "\n"
+               "private:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO\n"
+               "#endif\n"
+               "\n"
+               "private:\n"
+               "  int i;\n"
+               "#ifdef FOO\n"
+               "\n"
+               "private:\n"
+               "#endif\n"
+               "  int j;\n"
+               "};\n",
+               "struct foo {\n"
+               "#ifdef FOO\n"
+               "#endif\n"
+               "private:\n"
+               "  int i;\n"
+               "#ifdef FOO\n"
+               "private:\n"
+               "#endif\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Leave;
+  EXPECT_EQ("struct foo {\n"
+            "\n"
+            "private:\n"
+            "  void f() {}\n"
+            "\n"
+            "private:\n"
+            "  int i;\n"
+            "\n"
+            "protected:\n"
+            "  int j;\n"
+            "};\n",
+            format("struct foo {\n"
+                   "\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"
+               "private:\n"
+               "  int i;\n"
+               "protected:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  EXPECT_EQ("struct foo { /* comment */\n"
+            "\n"
+            "private:\n"
+            "  int i;\n"
+            "  // comment\n"
+            "\n"
+            "private:\n"
+            "  int j;\n"
+            "};\n",
+            format("struct foo { /* comment */\n"
+                   "\n"
+                   "private:\n"
+                   "  int i;\n"
+                   "  // comment\n"
+                   "\n"
+                   "private:\n"
+                   "  int j;\n"
+                   "};\n",
+                   Style));
+  verifyFormat("struct foo { /* comment */\n"
+               "private:\n"
+               "  int i;\n"
+               "  // comment\n"
+               "private:\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+  EXPECT_EQ("struct foo {\n"
+            "#ifdef FOO\n"
+            "#endif\n"
+            "\n"
+            "private:\n"
+            "  int i;\n"
+            "#ifdef FOO\n"
+            "\n"
+            "private:\n"
+            "#endif\n"
+            "  int j;\n"
+            "};\n",
+            format("struct foo {\n"
+                   "#ifdef FOO\n"
+                   "#endif\n"
+                   "\n"
+                   "private:\n"
+                   "  int i;\n"
+                   "#ifdef FOO\n"
+                   "\n"
+                   "private:\n"
+                   "#endif\n"
+                   "  int j;\n"
+                   "};\n",
+                   Style));
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO\n"
+               "#endif\n"
+               "private:\n"
+               "  int i;\n"
+               "#ifdef FOO\n"
+               "private:\n"
+               "#endif\n"
+               "  int j;\n"
+               "};\n",
+               Style);
+}
+
 TEST_F(FormatTest, FormatsArrays) {
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaa[aaaaaaaaaaaaaaaaaaaaaaaaa]\n"
                "                         [bbbbbbbbbbbbbbbbbbbbbbbbb] = c;");
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaa[aaaaaaaaaaa(aaaaaaaaaaaa)]\n"
                "                         [bbbbbbbbbbb(bbbbbbbbbbbb)] = c;");
   verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaa &&\n"
                "    aaaaaaaaaaaaaaaaaaa[aaaaaaaaaaaaa][aaaaaaaaaaaaa]) {\n}");
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
TEST_F(FormatTest, ParsesConfiguration) {
   CHECK_PARSE("BreakInheritanceList: AfterColon", BreakInheritanceList,
               FormatStyle::BILS_AfterColon);
   CHECK_PARSE("BreakInheritanceList: BeforeColon", BreakInheritanceList,
               FormatStyle::BILS_BeforeColon);
   // For backward compatibility:
   CHECK_PARSE("BreakBeforeInheritanceComma: true", BreakInheritanceList,
               FormatStyle::BILS_BeforeComma);
 
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
+  CHECK_PARSE("EmptyLineBeforeAccessModifier: Never",
+              EmptyLineBeforeAccessModifier, FormatStyle::ELBAMS_Never);
+  CHECK_PARSE("EmptyLineBeforeAccessModifier: Leave",
+              EmptyLineBeforeAccessModifier, FormatStyle::ELBAMS_Leave);
+  CHECK_PARSE("EmptyLineBeforeAccessModifier: LogicalBlock",
+              EmptyLineBeforeAccessModifier, FormatStyle::ELBAMS_LogicalBlock);
+  CHECK_PARSE("EmptyLineBeforeAccessModifier: Always",
+              EmptyLineBeforeAccessModifier, FormatStyle::ELBAMS_Always);
+
   Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
   CHECK_PARSE("AlignAfterOpenBracket: Align", AlignAfterOpenBracket,
               FormatStyle::BAS_Align);
   CHECK_PARSE("AlignAfterOpenBracket: DontAlign", AlignAfterOpenBracket,
               FormatStyle::BAS_DontAlign);
   CHECK_PARSE("AlignAfterOpenBracket: AlwaysBreak", AlignAfterOpenBracket,
               FormatStyle::BAS_AlwaysBreak);
   // For backward compatibility: