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@Darwins-iMac temp % cat f.cpp
class Foo {
class Bar {};
};
It will be formatted into:
darwin@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@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 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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
