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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

My 2 cents. Otherwise I agree with @MyDeveloperDay's comments.



================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1219-1228
+  if (Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+      PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
       RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)
     ++Newlines;
 
+  // Remove empty lines before access specifiers.
+  if (!Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
----------------
Given that the two `if`s handle the same construct, I think we should try to merge them to avoid duplication in conditions. Sth like:
```
if (PreviousLine && RootToken.isAccessSpecifier()) {
    if (... Style.Insert ... && ... ) {
    ...
    }
    else if ( (... !Style.Insert ... && ... ) {
    ...
    }
}
```


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1220
+  if (Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+      PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
       RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)
----------------
Just thinking out loud, but shouldn't we just check that `PreviousLine->Last->isNot(tok::l_brace)`?
Could you please add a test with comments before access modifiers?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1226
+  if (!Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+      PreviousLine->Last->isOneOf(tok::semi, tok::r_brace, tok::l_brace) &&
+      RootToken.isAccessSpecifier() && RootToken.NewlinesBefore > 1)
----------------
Is it really needed to check the type of `PreviousLine->Last`?


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1231-1232
   // Remove empty lines after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
     Newlines = std::min(1u, Newlines);
----------------
Please add tests with preprocessor directives *before* access modifiers too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846