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

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg marked an inline comment as done.
thezbyg 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)
----------------
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;
};
```


================
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 &&
----------------
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.


================
Comment at: clang/unittests/Format/FormatTest.cpp:8612-8625
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO\n"
+               "private:\n"
+               "#endif\n"
+               "  int i;\n"
+               "  int j;\n"
+               "}\n",
----------------
HazardyKnusperkeks wrote:
> Just curios, any reason why the struct is repeated? I don't seem to notice a difference.
> 
> And by the way, you are missing the `;` at the end of the definition, I don't know if that affects clang-format.
Some of the tests have equal expected and code values. I will update them to single parameter `verifyFormat()`.

clang-format does not seem to care about missing `;`, but I will add them as all other tests have them.


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

https://reviews.llvm.org/D93846