[PATCH] D104044: [clang-format] Fix the issue of no empty line after namespace

[Darwin Xu via Phabricator via cfe-commits]

darwin added a comment.

In D104044#2813515 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

> 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?

As for the code (original):

  darwin at Darwins-iMac temp % cat f.cpp 
  class Foo {
  
  
  
  
  class Bar {};
  };

It will be formatted into:

  darwin at Darwins-iMac temp % clang-format f.cpp -style="{MaxEmptyLinesToKeep: 1}"
  class Foo {
  
    class Bar {};
  };

I didn't see any problem here.

>   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 at 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 at 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 at 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 think it might be a good place to discuss the issue here.

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:

  darwin at Darwins-iMac temp % cat e.cpp
  namespace MyLibrary { class Tool {}; } 

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

  namespace MyLibrary
  {
  class Tool {};
  } 

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:

  


Repository:
  rZORG LLVM Github Zorg

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104044/new/

https://reviews.llvm.org/D104044