[clang-format] Fix the issue that empty lines being removed at the beginning of namespace

[darwin]

Summary

This is a bug fix of https://bugs.llvm.org/show_bug.cgi?id=50116

-----current behavior-----

darwin@darwin-ubuntu-04:~/temp$ cat b.cpp
namespace A{ int i; }

namespace B{


int j;


}

darwin@darwin-ubuntu-04:~/temp$ /home/darwin/projects/llvm-project/build/bin/clang-format  b.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterNamespace: true}}"
namespace A
{
int i;
}

namespace B
{
int j;

}

-----expected result-----

namespace A
{
int i;
}

namespace B
{

int j;

}

First revision only contains the change of the test code. I would like to have someone review the expected result. If it is OK. Then I can fix it.

Here is the output of the test case without fixing yet:

darwin@Darwins-iMac build % /Volumes/silo/Projects/open-source/llvm-project/build/tools/clang/unittests/Format/./FormatTests --gtest_filter=FormatTest.RemovesEmptyLines
Note: Google Test filter = FormatTest.RemovesEmptyLines
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.RemovesEmptyLines
/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:279: Failure
Expected equality of these values:
  "namespace N\n" "{\n" "\n" "int i;\n" "}"
    Which is: "namespace N\n{\n\nint i;\n}"
  format("namespace N\n" "{\n" "\n" "\n" "int    i;\n" "}", GoogleWrapBraceAfterNamespace)
    Which is: "namespace N\n{\nint i;\n}"
With diff:
@@ -1,5 @@
 namespace N
 {
-
 int i;
 }

/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:290: Failure
Expected equality of these values:
  "/* something */ namespace N\n" "{\n" "\n" "int i;\n" "}"
    Which is: "/* something */ namespace N\n{\n\nint i;\n}"
  format("/* something */ namespace N {\n" "\n" "\n" "int    i;\n" "}", GoogleWrapBraceAfterNamespace)
    Which is: "/* something */ namespace N\n{\nint i;\n}"
With diff:
@@ -1,5 @@
 /* something */ namespace N
 {
-
 int i;
 }

/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:302: Failure
Expected equality of these values:
  "inline namespace N\n" "{\n" "\n" "int i;\n" "}"
    Which is: "inline namespace N\n{\n\nint i;\n}"
  format("inline namespace N\n" "{\n" "\n" "\n" "int    i;\n" "}", GoogleWrapBraceAfterNamespace)
    Which is: "inline namespace N\n{\nint i;\n}"
With diff:
@@ -1,5 @@
 inline namespace N
 {
-
 int i;
 }

/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:313: Failure
Expected equality of these values:
  "/* something */ inline namespace N\n" "{\n" "\n" "int i;\n" "}"
    Which is: "/* something */ inline namespace N\n{\n\nint i;\n}"
  format("/* something */ inline namespace N\n" "{\n" "\n" "int    i;\n" "}", GoogleWrapBraceAfterNamespace)
    Which is: "/* something */ inline namespace N\n{\nint i;\n}"
With diff:
@@ -1,5 @@
 /* something */ inline namespace N
 {
-
 int i;
 }

/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:324: Failure
Expected equality of these values:
  "export namespace N\n" "{\n" "\n" "int i;\n" "}"
    Which is: "export namespace N\n{\n\nint i;\n}"
  format("export namespace N\n" "{\n" "\n" "int    i;\n" "}", GoogleWrapBraceAfterNamespace)
    Which is: "export namespace N\n{\nint i;\n}"
With diff:
@@ -1,5 @@
 export namespace N
 {
-
 int i;
 }

/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:345: Failure
Expected equality of these values:
  "namespace a\n" "{\n" "namespace b\n" "{\n" "\n" "class AA {};\n" "\n" "}  // namespace b\n" "}  // namespace a\n"
    Which is: "namespace a\n{\nnamespace b\n{\n\nclass AA {};\n\n}  // namespace b\n}  // namespace a\n"
  format("namespace a\n" "{\n" "namespace b\n" "{\n" "\n" "\n" "class AA {};\n" "\n" "\n" "}\n" "}\n", GoogleWrapBraceAfterNamespace)
    Which is: "namespace a\n{\nnamespace b\n{\nclass AA {};\n\n}  // namespace b\n}  // namespace a\n"
With diff:
@@ -3,5 @@
 namespace b
 {
-
 class AA {};
 

[  FAILED  ] FormatTest.RemovesEmptyLines (41 ms)
[----------] 1 test from FormatTest (41 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (41 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] FormatTest.RemovesEmptyLines

 1 FAILED TEST
darwin@Darwins-iMac build %

After the fix, all test cases have passed:

darwin@Darwins-iMac build % cmake --build . -j24 -t check-clang-unit ; echo '\007'
[  0%] Built target hmaptool
...
[100%] Built target ClangUnitTests
[100%] Running lit suite /Volumes/silo/Projects/open-source/llvm-project/clang/test/Unit

Testing Time: 245.60s
  Skipped:     3
  Passed : 14105
[100%] Built target check-clang-unit

[darwin]

Sorry, I may need some help here. It shows "Context not available.", how do I correct it?

[HazardyKnusperkeks]

In D104044#2810909, @darwin wrote:

Sorry, I may need some help here. It shows "Context not available.", how do I correct it?

There are multiple ways: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
I use git diff HEAD~1 -U999999 > mypatch.patch

For your tests: You want to keep an empty line after the now wrapped {, did I understand that correctly?
Is that bound to the google style, i.e. does it not happen with LLVM style?

[darwin]

In D104044#2811268, @HazardyKnusperkeks wrote:

In D104044#2810909, @darwin wrote:

Sorry, I may need some help here. It shows "Context not available.", how do I correct it?

There are multiple ways: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
I use git diff HEAD~1 -U999999 > mypatch.patch

For your tests: You want to keep an empty line after the now wrapped {, did I understand that correctly?
Is that bound to the google style, i.e. does it not happen with LLVM style?

Oh, now I remember, I forget to use this command. Thank you.

About the issue, let me explain it. It isn't bound to the google style or LLVM style either, since both of them keep the first brace at the same line of the namespace.

Let's see this example:

darwin@darwin-ubuntu-04:~/temp$ cat b.cpp
namespace A{ int i; }

namespace B{


int j;


}

darwin@darwin-ubuntu-04:~/temp$ /home/darwin/projects/llvm-project/build/bin/clang-format  b.cpp -style="{BasedOnStyle: google}"
namespace A {
int i;
}

namespace B {

int j;

}

You can see, if there isn't an empty line, clang-format doesn't add one. And if there are some extra lines, clang-format will keep just one line. This is expected.

But if I use set BraceWrapping.AfterNamespace to true, the thing is different, look this:

darwin@darwin-ubuntu-04:~/temp$ cat b.cpp
namespace A{ int i; }

namespace B{


int j;


}

darwin@darwin-ubuntu-04:~/temp$ /home/darwin/projects/llvm-project/build/bin/clang-format  b.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterNamespace: true}}"
namespace A
{
int i;
}

namespace B
{
int j;

}

There isn't an empty line between the { and the int j;, but in the previous example, there is an empty line.

But later I found out that if I set KeepEmptyLinesAtTheStartOfBlocks to true, I will get the expect result:

darwin@darwin-ubuntu-04:~/temp$ cat b.cpp
namespace A{ int i; }

namespace B{


int j;


}

darwin@darwin-ubuntu-04:~/temp$ /home/darwin/projects/llvm-project/build/bin/clang-format  b.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterNamespace: true}, KeepEmptyLinesAtTheStartOfBlocks: true}"
namespace A
{
int i;
}

namespace B
{

int j;

}

So, this might not be a very critical issue. But still, I think clang-format isn't working correctly and should be fixed.

[MyDeveloperDay]

Just so I'm clear are you proposing that a newlines should always be added or that single blank lines should be ignored? I can't tell if the bug is that the line isn't being removed or sometimes not being added, either way there will be someone who wants it the other way around correct?

[darwin]

In D104044#2812612, @MyDeveloperDay wrote:

Just so I'm clear are you proposing that a newlines should always be added or that single blank lines should be ignored? I can't tell if the bug is that the line isn't being removed or sometimes not being added, either way there will be someone who wants it the other way around correct?

If there is no linefeed at all, or there is only one line feed. The clang-format works fine.

The issue is, if there are empty lines between the line of namespace and the first line of statement, those empty lines will be removed.

For example, I want to to have code like this:

01 namespace MyLibrary
02 {
03
04 class Tool
05 {
06 };
07 
08 }

There is an empty line 03.

If I format this with clang-format, I will got this:

01 namespace MyLibrary
02 {
03 class Tool
04 {
05 };
06 
07 }

The empty line between line 02 and line 03 is removed. This asymmetry is very annoying.

[MyDeveloperDay]

Devils advocate how is this any different from

class Foo {

class Bar {} ;
}

};

This would become

class Foo {
   class Bar {};
};

i.e. its going to remove the extra lines, just asking so we can understand if the removal of the line is the error or the fact it doesn't remove the line in the first place?

[MyDeveloperDay]

Do we need a set options for when we want to insert/retain/add a newline after various constructs? frankly I've been mulling over the concept of adding a

NewLinesBetweenFunctions: 1

I personally don't like code written like this as I find it hard to read, I'd like to be able to mandate a single line between functions

void foo()
{
...
}
void bar()
{
...
}
void foobar()
{
...
}

I prefer when its written as:

void foo()
{
...
}

void bar()
{
...
}

void foobar()
{
...
}

Maybe we even need a more generalised mechanism that would allow alot of flexibility letting people control their own specific style.

EmptyLineInsertionStyle: Custom
      AfterNameSpaceOpeningBrace: 1
      BeforeNameSpaceClosingBrace: 1
      BetweenFunctions: 2
      AfterClassOpeningBrace: 1
      BeforeClassClosingBrace: 1
      AfterAccessModifier : 1
      BeforeAccessModifier: 1

[MyDeveloperDay]

We should have perhaps though about this when we added EmptyLineAfterAccessModifier and EmptyLineBeforeAccessModifier

[darwin]

In D104044#2813491, @MyDeveloperDay wrote:

Devils advocate how is this any different from

class Foo {

class Bar {} ;
}

};

This would become

class Foo {
   class Bar {};
};

i.e. its going to remove the extra lines, just asking so we can understand if the removal of the line is the error or the fact it doesn't remove the line in the first place?

It is different, the issue I mentioned is about the empty lines after the namespace. An empty line after the namespace could make the code easier to read, this is what clang-format is all about.

As for class, clang-format always removes the empty lines in class:

darwin@Darwins-iMac temp % cat f.cpp 
class Foo {

class Bar {} ;

};
darwin@Darwins-iMac temp % clang-format f.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterClass: true}}" 
class Foo
{
  class Bar
  {
  };
};
darwin@Darwins-iMac temp % clang-format f.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterClass: false}}"
class Foo {
  class Bar {};
};

Except for when KeepEmptyLinesAtTheStartOfBlocks is true:

darwin@Darwins-iMac temp % clang-format f.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterClass: false}, KeepEmptyLinesAtTheStartOfBlocks: true}"
class Foo {

  class Bar {};
};

[MyDeveloperDay]

My point being there is inconsistency between how different types of blocks of code are handled, and rather than trying to fix another corner case maybe we should take a more holistic approach, all these KeepEmptyLines and EmptyLineAfterXXX options and what you'll need in order to fix this issue are all addressing what is effectively the same issue, and that is that the addition and/or removal of empty lines is a little hit or miss depending on your combination and permutation of settings and the type of block

I personally would prefer we took a step back and asked ourselves if we are really facing a bug here or just different people desiring different functionality?

Whatever the rules are for an inner class, I don't particularly see they are much different for a class in a namespace (which I why I picked that example to demonstrate the point), we won't resolve that inconsistency in a way that will satisfy everyone without having a more powerful mechanism.

If you are thinking you want to just fix your bug then I'd be saying that it SHOULD remove the empty lines (including the one prior to the } // namespace MyLibrary, having said that I'm slightly struggling to understand why

class Foo {




class Bar {};
};

isn't conforming to the setting of MaxEmptyLinesToKeep if I set it to 1 where

namespace MyLibrary {

class Tool {};
}  // namespace MyLibrary

is

i.e. set MaxEmptyLinesToKeep to 0 and

it gets formatted as:

namespace MyLibrary {
class Tool {};
}  // namespace MyLibrary

We don't normally just review tests, but what were you thinking the fix should be?

[HazardyKnusperkeks]

Going the full way, to fix the number of empty lines after/before/between elements would be real nice. But even nicer would be if you can state a range.

But I think all those proposed options should not be added in one go.

In D104044#2812399, @darwin wrote:

About the issue, let me explain it. It isn't bound to the google style or LLVM style either, since both of them keep the first brace at the same line of the namespace.

Then I would like to use the LLVM style in the tests, otherwise it suggests that the issue is a result of using google style.

[darwin]

In D104044#2813515, @MyDeveloperDay wrote:

Do we need a set options for when we want to insert/retain/add a newline after various constructs? frankly I've been mulling over the concept of adding a

NewLinesBetweenFunctions: 1

I personally don't like code written like this as I find it hard to read, I'd like to be able to mandate a single line between functions

void foo()
{
...
}
void bar()
{
...
}
void foobar()
{
...
}

I prefer when its written as:

void foo()
{
...
}

void bar()
{
...
}

void foobar()
{
...
}

Maybe we even need a more generalised mechanism that would allow alot of flexibility letting people control their own specific style.

EmptyLineInsertionStyle: Custom
      AfterNameSpaceOpeningBrace: 1
      BeforeNameSpaceClosingBrace: 1
      BetweenFunctions: 2
      AfterClassOpeningBrace: 1
      BeforeClassClosingBrace: 1
      AfterAccessModifier : 1
      BeforeAccessModifier: 1

I totally agree with you on this part, but I think this is a new feature requirement. Maybe we can open a new bug and set the "Severity" to "enhancement".

In D104044#2813794, @MyDeveloperDay wrote:

My point being there is inconsistency between how different types of blocks of code are handled, and rather than trying to fix another corner case maybe we should take a more holistic approach, all these KeepEmptyLines and EmptyLineAfterXXX options and what you'll need in order to fix this issue are all addressing what is effectively the same issue, and that is that the addition and/or removal of empty lines is a little hit or miss depending on your combination and permutation of settings and the type of block

I agree that we should use a holistic approach to solve the problem as long as we can, but I think the namespace is different than the class based on those reasons:

• We indent in the class scope, but we almost never indent in the namespace scope. (I've checked all the predefined styles)
• The life-cycles of the objects in the class scope are associated with the class scope, but the life-cycles of the objects in a namespace is always global. (The life-cycle is not relevant to the format, but this is an evidence that namespace and class are different.)

I personally would prefer we took a step back and asked ourselves if we are really facing a bug here or just different people desiring different functionality?

My reason for this being a bug is very simple. If my original code is like this (original):

01 namespace B
02 {
03
04
04 int j;
05
06
07 }

Then I want the code to be formatted like this (expected):

01 namespace B
02 {
03
04 int j;
05
06 }

Not like this (actual):

01 namespace B
02 {
03 int j;
04
05 }

Whatever the rules are for an inner class, I don't particularly see they are much different for a class in a namespace (which I why I picked that example to demonstrate the point), we won't resolve that inconsistency in a way that will satisfy everyone without having a more powerful mechanism.

If you are thinking you want to just fix your bug then I'd be saying that it SHOULD remove the empty lines (including the one prior to the } // namespace MyLibrary, having said that I'm slightly struggling to understand why

class Foo {




class Bar {};
};

isn't conforming to the setting of MaxEmptyLinesToKeep if I set it to 1 where

"it SHOULD remove the empty lines (including the one prior to the } // namespace MyLibrary", I don't get what you mean here, can you give me an example? Like, what is the original code, what do you expect, and what is the actual result?

namespace MyLibrary {

class Tool {};
}  // namespace MyLibrary

is

i.e. set MaxEmptyLinesToKeep to 0 and

it gets formatted as:

namespace MyLibrary {
class Tool {};
}  // namespace MyLibrary

The behavior is correct in this case. Notice the { is on the same line as the namespace. If I set AfterNamespace as true, then the behavior is different.

Orignal code, there are two empty lines before and after class Tool {};:

darwin@Darwins-iMac temp % cat e.cpp 
01 namespace MyLibrary {
02
03 
04 class Tool {};
05 
06 
07 }

If I format it with AfterNamespace as true and MaxEmptyLinesToKeep as 0, all those empty lines are removed, which is correct:

darwin@Darwins-iMac temp % clang-format e.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterNamespace: true}, MaxEmptyLinesToKeep: 0}"
01 namespace MyLibrary
02 {
03 class Tool {};
04 }  // namespace MyLibrary

If I format with AfterNamespace as true and MaxEmptyLinesToKeep as 1, the empty lines after class Tool {}; are reduced to 1, which is correct, but the empty lines before class Tool {}; is still zero, which I think is wrong:

darwin@Darwins-iMac temp % clang-format e.cpp -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterNamespace: true}, MaxEmptyLinesToKeep: 1}"
01 namespace MyLibrary
02 {
03 class Tool {};
04 
05 }  // namespace MyLibrary

We don't normally just review tests, but what were you thinking the fix should be?

The reason I started with a test was because no one seemed to be reviewing the bug I reported: https://bugs.llvm.org/show_bug.cgi?id=50116. So, I thought I might get some feedback if I put up a test case first.

In my opinion, if there is no empty line in the namespace, clang-format should keep it this way. For example, if the original code is:

01 namespace MyLibrary { class Tool {}; }

The formatted code (with AfterNamespace as true and MaxEmptyLinesToKeep as 1) is expected to be like this:

01 namespace MyLibrary
02 {
03 class Tool {};
04 }

If there are empty lines in the beginning and the ending of the namespace, then clang-format should reduce them in a consistent way. For example, if the original code is:

01 namespace MyLibrary {
02 
03 
04 class Tool {};
05 
06 
07 }

The formatted code (with AfterNamespace as true and MaxEmptyLinesToKeep as 1) is expected to be like this:

01 namespace MyLibrary
02 {
03 
04 class Tool {};
05 
06 }

[darwin]

In D104044#2814016, @HazardyKnusperkeks wrote:

Going the full way, to fix the number of empty lines after/before/between elements would be real nice. But even nicer would be if you can state a range.

But I think all those proposed options should not be added in one go.

Yes, adding a new option is a new feature requirement. What I am trying to do here is to keep the clang-format behaving reasonably.

In D104044#2812399, @darwin wrote:

About the issue, let me explain it. It isn't bound to the google style or LLVM style either, since both of them keep the first brace at the same line of the namespace.

Then I would like to use the LLVM style in the tests, otherwise it suggests that the issue is a result of using google style.

Wouldn't this suggest that this issue is a result of using LLVM style?

[HazardyKnusperkeks]

In D104044#2812399, @darwin wrote:

About the issue, let me explain it. It isn't bound to the google style or LLVM style either, since both of them keep the first brace at the same line of the namespace.

Then I would like to use the LLVM style in the tests, otherwise it suggests that the issue is a result of using google style.

Wouldn't this suggest that this issue is a result of using LLVM style?

Not in my opinion, because all the tests I've seen and worked with are LLVM style, because that's the default. But use what ever style you want.

[MyDeveloperDay]

I think we can agree its complicated, how about you take your unit tests and show us what the "code change" looks like to fix the bug

If that gets overly convoluted then perhaps we can bring the idea forward of a more generalised approach.

[darwin]

In D104044#2816397, @MyDeveloperDay wrote:

I think we can agree its complicated, how about you take your unit tests and show us what the "code change" looks like to fix the bug

If that gets overly convoluted then perhaps we can bring the idea forward of a more generalised approach.

Thanks, I can do that.

[darwin]

Updated with the fix code.

Look into the code, I am pretty sure this is a bug, it is about the logic of the parameter KeepEmptyLinesAtTheStartOfBlocks.

When KeepEmptyLinesAtTheStartOfBlocks is false, it will remove the empty lines at the start of the block, and in namespace it is an exception. So the empty lines will be kept inside namespace.

The problem is, it can only handle the situation in which the namespace and the { are on the same line. If BraceWrapping.AfterNamespace is true, it will cause the namespace and the { to be separated into different lines. The original code overlooked this situation:

// Remove empty lines after "{".
if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
    PreviousLine->Last->is(tok::l_brace) &&
    !PreviousLine->startsWithNamespace() &&
    !startsExternCBlock(*PreviousLine))
  Newlines = 1;

As you can see, it only checks if previous line starts with namespace.

The solution is a bit of tricky, we need to check not only the previous line, but also the line before the previous line. There isn't an easy way to get this. So I added a new parameter PrevPrevLine to the UnwrappedLineFormatter::formatFirstToken() function. I have to admit that it isn't an elegant solution. Let me know if there is a better way to do so.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

281

What does

namespace N  { /* comment */

or

namespace N   /* comment */ {

do

[darwin]

clang/unittests/Format/FormatTest.cpp

281

It works fine: (with -style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: {AfterNamespace: true}}")

namespace A /* comment */ { class B {} }

namespace A {/* comment */ class B {} }

namespace A { /* comment */


 class B {}


  }

namespace A/* comment */ {


 class B {}


  }

Is formatted like this:

namespace A /* comment */
{
class B {}
}  // namespace A

namespace A
{ /* comment */
class B {}
}  // namespace A

namespace A
{ /* comment */

class B {}

}  // namespace A

namespace A /* comment */
{

class B {}

}  // namespace A

I will update the test case too.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

280

I'm not sure I understand this.. why is this not

namespace N
{
int i;
}

[darwin]

clang/unittests/Format/FormatTest.cpp

280

Because a namespace is different than a class or struct.

Please try this with chang-format:

darwin@Darwins-iMac Desktop % cat a.cpp 
namespace N {


int i;


}
darwin@Darwins-iMac Desktop % clang-format a.cpp -style="{BasedOnStyle: google}"
namespace N {

int i;

}

As you can see, clang-format removes the extra lines but still keeps one line at the beginning and the end of the namespace. This is the original behavior, I think it is very reasonable.

[MyDeveloperDay]

LGTM

[darwin]

Add new test cases according to the comments.

[darwin]

And can someone commit it for me? I don't have the right to push it yet. Or let me know how to apply? Thank you very much.

[HazardyKnusperkeks]

In D104044#2842726, @darwin wrote:

And can someone commit it for me? I don't have the right to push it yet. Or let me know how to apply? Thank you very much.

https://llvm.org/docs/DeveloperPolicy.html?highlight=chris#obtaining-commit-access

[darwin]

In D104044#2842757, @HazardyKnusperkeks wrote:

In D104044#2842726, @darwin wrote:

And can someone commit it for me? I don't have the right to push it yet. Or let me know how to apply? Thank you very much.

https://llvm.org/docs/DeveloperPolicy.html?highlight=chris#obtaining-commit-access

Thanks, guess I still need you to commit it for me this time (User: Darwin Xu, Email: [e-mail]), I cannot get the access any time soon. Thank you.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineFormatter.cpp

1282

Nit:I do think at some point we need to have some sort of option for controlling these empty lines,

`
EmptyLine:
         AfterNamespace: 1
         BeforeAccessModifier: 1
         AfterAccessModifier: 1
         BetweenFunctions: 1
         ...
`

[darwin]

clang/lib/Format/UnwrappedLineFormatter.cpp

1282

Yes, maybe we can open a bug for it.

[pem]

I am looking forward for this EmptyLineInsertion feature.

In D104044#2813515, @MyDeveloperDay wrote:

Do we need a set options for when we want to insert/retain/add a newline after various constructs? frankly I've been mulling over the concept of adding a

NewLinesBetweenFunctions: 1

I personally don't like code written like this as I find it hard to read, I'd like to be able to mandate a single line between functions

void foo()
{
...
}
void bar()
{
...
}
void foobar()
{
...
}

I prefer when its written as:

void foo()
{
...
}

void bar()
{
...
}

void foobar()
{
...
}

Maybe we even need a more generalised mechanism that would allow alot of flexibility letting people control their own specific style.

EmptyLineInsertionStyle: Custom
      AfterNameSpaceOpeningBrace: 1
      BeforeNameSpaceClosingBrace: 1
      BetweenFunctions: 2
      AfterClassOpeningBrace: 1
      BeforeClassClosingBrace: 1
      AfterAccessModifier : 1
      BeforeAccessModifier: 1

clang/lib/Format/UnwrappedLineFormatter.h

unsigned format(const SmallVectorImpl<AnnotatedLine *> &Lines,
                   bool FixBadIndentation = false, unsigned FirstStartColumn = 0,
                   unsigned NextStartColumn = 0, unsigned LastStartColumn = 0);
 
 private:
   /// Add a new line and the required indent before the first Token
   /// of the \c UnwrappedLine if there was no structural parsing error.
   void formatFirstToken(const AnnotatedLine &Line,
                         const AnnotatedLine *PreviousLine,
+                        const AnnotatedLine *PrevPrevLine,
                         const SmallVectorImpl<AnnotatedLine *> &Lines,
                         unsigned Indent, unsigned NewlineIndent);
 
   /// Returns the column limit for a line, taking into account whether we
   /// need an escaped newline due to a continued preprocessor directive.
   unsigned getColumnLimit(bool InPPDirective,
                           const AnnotatedLine *NextLine) const;
 

clang/lib/Format/UnwrappedLineFormatter.cpp

unsigned UnwrappedLineFormatter::format(
   auto CacheIt = PenaltyCache.find(CacheKey);
   if (DryRun && CacheIt != PenaltyCache.end())
     return CacheIt->second;
 
   assert(!Lines.empty());
   unsigned Penalty = 0;
   LevelIndentTracker IndentTracker(Style, Keywords, Lines[0]->Level,
                                    AdditionalIndent);
+  const AnnotatedLine *PrevPrevLine = nullptr;
   const AnnotatedLine *PreviousLine = nullptr;
   const AnnotatedLine *NextLine = nullptr;
 
   // The minimum level of consecutive lines that have been formatted.
   unsigned RangeMinLevel = UINT_MAX;
 
   bool FirstLine = true;
   for (const AnnotatedLine *Line =
if (ShouldFormat && TheLine.Type == LT_Invalid && Status) {
       Status->FormatComplete = false;
       Status->Line =
           SourceMgr.getSpellingLineNumber(TheLine.First->Tok.getLocation());
     }
 
     if (ShouldFormat && TheLine.Type != LT_Invalid) {
       if (!DryRun) {
         bool LastLine = Line->First->is(tok::eof);
-        formatFirstToken(TheLine, PreviousLine, Lines, Indent,
+        formatFirstToken(TheLine, PreviousLine, PrevPrevLine, Lines, Indent,
                          LastLine ? LastStartColumn : NextStartColumn + Indent);
       }
 
       NextLine = Joiner.getNextMergedLine(DryRun, IndentTracker);
       unsigned ColumnLimit = getColumnLimit(TheLine.InPPDirective, NextLine);
       bool FitsIntoOneLine =
           TheLine.Last->TotalLength + Indent <= ColumnLimit ||
           (TheLine.Type == LT_ImportStatement &&
if (ShouldFormat && TheLine.Type != LT_Invalid) {
       if (StartsNewLine)
         IndentTracker.adjustToUnmodifiedLine(TheLine);
       if (!DryRun) {
         bool ReformatLeadingWhitespace =
             StartsNewLine && ((PreviousLine && PreviousLine->Affected) ||
                               TheLine.LeadingEmptyLinesAffected);
         // Format the first token.
         if (ReformatLeadingWhitespace)
-          formatFirstToken(TheLine, PreviousLine, Lines,
+          formatFirstToken(TheLine, PreviousLine, PrevPrevLine, Lines,
                            TheLine.First->OriginalColumn,
                            TheLine.First->OriginalColumn);
         else
           Whitespaces->addUntouchableToken(*TheLine.First,
                                            TheLine.InPPDirective);
 
         // Notify the WhitespaceManager about the unchanged whitespace.
         for (FormatToken *Tok = TheLine.First->Next; Tok; Tok = Tok->Next)
           Whitespaces->addUntouchableToken(*Tok, TheLine.InPPDirective);
       }
       NextLine = Joiner.getNextMergedLine(DryRun, IndentTracker);
       RangeMinLevel = UINT_MAX;
     }
     if (!DryRun)
       markFinalized(TheLine.First);
+    PrevPrevLine = PreviousLine;
     PreviousLine = &TheLine;
   }
   PenaltyCache[CacheKey] = Penalty;
   return Penalty;
 }
 
 void UnwrappedLineFormatter::formatFirstToken(
     const AnnotatedLine &Line, const AnnotatedLine *PreviousLine,
+    const AnnotatedLine *PrevPrevLine,
     const SmallVectorImpl<AnnotatedLine *> &Lines, unsigned Indent,
     unsigned NewlineIndent) {
   FormatToken &RootToken = *Line.First;
   if (RootToken.is(tok::eof)) {
     unsigned Newlines = std::min(RootToken.NewlinesBefore, 1u);
     unsigned TokenIndent = Newlines ? NewlineIndent : 0;
     Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent,
                                    TokenIndent);
if (Newlines == 0 && !RootToken.IsFirst)
     Newlines = 1;
   if (RootToken.IsFirst && !RootToken.HasUnescapedNewline)
     Newlines = 0;
 
   // Remove empty lines after "{".
   if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
       PreviousLine->Last->is(tok::l_brace) &&
       !PreviousLine->startsWithNamespace() &&
+      !(PrevPrevLine && PrevPrevLine->startsWithNamespace() &&
+        PreviousLine->startsWith(tok::l_brace)) &&
       !startsExternCBlock(*PreviousLine))
     Newlines = 1;
 
   // Insert or remove empty line before access specifiers.
   if (PreviousLine && RootToken.isAccessSpecifier()) {
     switch (Style.EmptyLineBeforeAccessModifier) {
     case FormatStyle::ELBAMS_Never:
       if (Newlines > 1)
         Newlines = 1;
       break;

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("extern /**/ \"C\" /**/ {\n"
             "int i;\n"
             "}",
             format("extern /**/ \"C\" /**/ {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
 
+  auto CustomStyle = clang::format::getLLVMStyle();
+  CustomStyle.BreakBeforeBraces = clang::format::FormatStyle::BS_Custom;
+  CustomStyle.BraceWrapping.AfterNamespace = true;
+  CustomStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
+  EXPECT_EQ("namespace N\n"
+            "{\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("namespace N\n"
+                   "{\n"
+                   "\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   CustomStyle));
+  EXPECT_EQ("/* something */ namespace N\n"
+            "{\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("/* something */ namespace N {\n"
+                   "\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   CustomStyle));
+  EXPECT_EQ("inline namespace N\n"
+            "{\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("inline namespace N\n"
+                   "{\n"
+                   "\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   CustomStyle));
+  EXPECT_EQ("/* something */ inline namespace N\n"
+            "{\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("/* something */ inline namespace N\n"
+                   "{\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   CustomStyle));
+  EXPECT_EQ("export namespace N\n"
+            "{\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("export namespace N\n"
+                   "{\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   CustomStyle));
+  EXPECT_EQ("namespace a\n"
+            "{\n"
+            "namespace b\n"
+            "{\n"
+            "\n"
+            "class AA {};\n"
+            "\n"
+            "} // namespace b\n"
+            "} // namespace a\n",
+            format("namespace a\n"
+                   "{\n"
+                   "namespace b\n"
+                   "{\n"
+                   "\n"
+                   "\n"
+                   "class AA {};\n"
+                   "\n"
+                   "\n"
+                   "}\n"
+                   "}\n",
+                   CustomStyle));
+  EXPECT_EQ("namespace A /* comment */\n"
+            "{\n"
+            "class B {}\n"
+            "} // namespace A",
+            format("namespace A /* comment */ { class B {} }", CustomStyle));
+  EXPECT_EQ("namespace A\n"
+            "{ /* comment */\n"
+            "class B {}\n"
+            "} // namespace A",
+            format("namespace A {/* comment */ class B {} }", CustomStyle));
+  EXPECT_EQ("namespace A\n"
+            "{ /* comment */\n"
+            "\n"
+            "class B {}\n"
+            "\n"
+            ""
+            "} // namespace A",
+            format("namespace A { /* comment */\n"
+                   "\n"
+                   "\n"
+                   "class B {}\n"
+                   "\n"
+                   "\n"
+                   "}",
+                   CustomStyle));
+  EXPECT_EQ("namespace A /* comment */\n"
+            "{\n"
+            "\n"
+            "class B {}\n"
+            "\n"
+            "} // namespace A",
+            format("namespace A/* comment */ {\n"
+                   "\n"
+                   "\n"
+                   "class B {}\n"
+                   "\n"
+                   "\n"
+                   "}",
+                   CustomStyle));
+
   // ...but do keep inlining and removing empty lines for non-block extern "C"
   // functions.
   verifyFormat("extern \"C\" int f() { return 42; }", getGoogleStyle());
   EXPECT_EQ("extern \"C\" int f() {\n"
             "  int i = 42;\n"
             "  return i;\n"
             "}",
             format("extern \"C\" int f() {\n"