[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

1

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

1219–1228

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 ... && ... ) {
    ...
    }
}

1220

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?

1226

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

1231–1232

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

[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...).

2370

Here it is sorted.

clang/lib/Format/Format.cpp

414

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

8544–8556 ↗

(On Diff #313863)

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

1220

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

8544–8556 ↗

(On Diff #313863)

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

8544–8556 ↗

(On Diff #313863)

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

1545 ↗

(On Diff #313973)

Full stop at the end of the phrase.

[MyDeveloperDay]

clang/include/clang/Format/Format.h

1363

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

8544 ↗

(On Diff #314129)

any reason this can't be verifyFormat?

8565 ↗

(On Diff #314129)

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

1225

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

1225

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

1225

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.

1226–1227

A tab?

clang/unittests/Format/FormatTest.cpp

8612–8625 ↗

(On Diff #314129)

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

1226–1227

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

1225

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;
};

1226–1227

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

8612–8625 ↗

(On Diff #314129)

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

1225

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.

1226–1227

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 ↗

(On Diff #316511)

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 ↗

(On Diff #316660)

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/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;
 
+  /// If true, the empty line is inserted before access modifiers
+  /// \code
+  ///    true:                                  false:
+  ///    struct foo {                   vs.     struct foo {
+  ///        int i;                                 int i;
+  ///                                           private:
+  ///    private:                                   int j;
+  ///        int j;                             }
+  ///    }
+  /// \endcode
+  bool InsertEmptyLineBeforeAccessModifier;
+
   /// Different styles for aligning after open brackets.
   enum BracketAlignmentStyle {
     /// Align parameters on the open bracket, e.g.:
     /// \code
     ///   someLongFunction(argument1,
     ///                    argument2);
     /// \endcode
     BAS_Align,

   /// 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;
 
   /// 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.
   ///
   /// NOTE: This is an experimental flag, that might go away or be renamed. Do
return AccessModifierOffset == R.AccessModifierOffset &&
                R.IncludeStyle.IncludeIsMainSourceRegex &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentCaseBlocks == R.IndentCaseBlocks &&
            IndentGotoLabels == R.IndentGotoLabels &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentExternBlock == R.IndentExternBlock &&
            IndentWidth == R.IndentWidth && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
+           InsertEmptyLineBeforeAccessModifier ==
+               R.InsertEmptyLineBeforeAccessModifier &&
            JavaImportGroups == R.JavaImportGroups &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&
            KeepEmptyLinesAtTheStartOfBlocks ==
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&

clang/lib/Format/Format.cpp

 };
 
 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);
   }
 };
 
 template <>
 struct ScalarEnumerationTraits<FormatStyle::IndentExternBlockStyle> {
   static void enumeration(IO &IO, FormatStyle::IndentExternBlockStyle &Value) {
     IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
     IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
     IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
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("InsertEmptyLineBeforeAccessModifier",
+                   Style.InsertEmptyLineBeforeAccessModifier);
     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);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
+  LLVMStyle.InsertEmptyLineBeforeAccessModifier = true;
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
   LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;

clang/lib/Format/UnwrappedLineFormatter.cpp

void UnwrappedLineFormatter::formatFirstToken(
   // 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) &&
+  if (Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+      PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
       RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)
     ++Newlines;
 
+  // Remove empty lines before access specifiers.
+  if (!Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+      PreviousLine->Last->isOneOf(tok::semi, tok::r_brace, tok::l_brace) &&
+      RootToken.isAccessSpecifier() && RootToken.NewlinesBefore > 1)
+    Newlines = 1;
+
   // 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, InsertEmptyLineBeforeAccessModifier: true}" -lines=1:10 \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, InsertEmptyLineBeforeAccessModifier: false}" -lines=11:32 \
+// 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 i
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct foo3 {
+  int i;
+
+private:
+  int j;
+}
+
+// CHECK: struct bar3
+// CHECK-NEXT: {{^private:$}}
+// CHECK: }
+struct bar3 {
+
+private:
+  int i;
+  int j;
+}