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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits