[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:
> 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.


================
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 &&
----------------
A tab?


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


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

https://reviews.llvm.org/D93846