[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1221
+    if (Style.EmptyLineBeforeAccessModifier &&
+        PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+        RootToken.NewlinesBefore == 1)
----------------
thezbyg wrote:
> HazardyKnusperkeks wrote:
> > thezbyg wrote:
> > > MyDeveloperDay wrote:
> > > > 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?
> > > 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;
> > > }
> > > ```
> > 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.
> 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;
> };
> ```
> 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.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1222-1223
+        PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+        RootToken.NewlinesBefore == 1)
+      ++Newlines;
+    else if (!Style.EmptyLineBeforeAccessModifier &&
----------------
thezbyg wrote:
> MyDeveloperDay wrote:
> > HazardyKnusperkeks wrote:
> > > A tab?
> > 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
> > 
> > 
> > 
> > 
> 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.
I would assume so. I was just irritated by the >>. Everything is fine here.


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

https://reviews.llvm.org/D93846