Page MenuHomePhabricator

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

Authored by darwin on Thu, Jun 10, 9:47 AM.

Details

Reviewers
gkistanova
HazardyKnusperkeks
curdeius
MyDeveloperDay
Group Reviewers
Restricted Project
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 %

Diff Detail

Unit TestsFailed

TimeTest
30 msx64 debian > Clang-Unit.Format/_/FormatTests::FormatTest.RemovesEmptyLines
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/Format/./FormatTests --gtest_filter=FormatTest.RemovesEmptyLines
12,640 msx64 debian > libarcher.races::lock-unrelated.c
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c
70 msx64 windows > Clang-Unit.Format/_/FormatTests_exe::FormatTest.RemovesEmptyLines
Script: -- C:\ws\w4\llvm-project\premerge-checks\build\tools\clang\unittests\Format\.\FormatTests.exe --gtest_filter=FormatTest.RemovesEmptyLines

Event Timeline

darwin requested review of this revision.Thu, Jun 10, 9:47 AM
darwin created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 10, 9:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
darwin changed the repository for this revision from rG LLVM Github Monorepo to rZORG LLVM Github Zorg.Thu, Jun 10, 9:48 AM

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

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?

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.

darwin edited the summary of this revision. (Show Details)Thu, Jun 10, 10:14 PM
darwin edited the summary of this revision. (Show Details)Thu, Jun 10, 11:27 PM

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?

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.

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 added a comment.EditedFri, Jun 11, 9:01 AM

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 added a comment.EditedFri, Jun 11, 9:03 AM

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

darwin added a comment.EditedFri, Jun 11, 9:54 AM

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

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?

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.

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 added a comment.EditedSun, Jun 13, 6:21 AM

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".

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 }

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.

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?

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.

darwin retitled this revision from [clang-format] Fix the issue of no empty line after namespace to [clang-format] Fix the issue that empty lines being removed at the beginning of namespace.Sun, Jun 13, 10:25 AM

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.

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.