diff --git a/clang/lib/Format/DefinitionBlockSeparator.cpp b/clang/lib/Format/DefinitionBlockSeparator.cpp --- a/clang/lib/Format/DefinitionBlockSeparator.cpp +++ b/clang/lib/Format/DefinitionBlockSeparator.cpp @@ -31,15 +31,16 @@ void DefinitionBlockSeparator::separateBlocks( SmallVectorImpl &Lines, tooling::Replacements &Result) { + const bool IsNeverStyle = + Style.SeparateDefinitionBlocks == FormatStyle::SDS_Never; auto LikelyDefinition = [this](const AnnotatedLine *Line) { - if (Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) + if ((Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) || + Line->startsWithNamespace()) return true; FormatToken *CurrentToken = Line->First; while (CurrentToken) { - if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct, - tok::kw_namespace, tok::kw_enum) || - (Style.Language == FormatStyle::LK_JavaScript && - CurrentToken->TokenText == "function")) + if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct, tok::kw_enum) || + (Style.isJavaScript() && CurrentToken->TokenText == "function")) return true; CurrentToken = CurrentToken->Next; } @@ -54,11 +55,14 @@ Env.getSourceManager().getBufferData(Env.getFileID()), Style.UseCRLF) : Style.UseCRLF); - for (unsigned I = 0; I < Lines.size(); I++) { + for (unsigned I = 0; I < Lines.size(); ++I) { const auto &CurrentLine = Lines[I]; + if (CurrentLine->InPPDirective) + continue; FormatToken *TargetToken = nullptr; AnnotatedLine *TargetLine; auto OpeningLineIndex = CurrentLine->MatchingOpeningBlockLineIndex; + AnnotatedLine *OpeningLine = nullptr; const auto InsertReplacement = [&](const int NewlineToInsert) { assert(TargetLine); assert(TargetToken); @@ -72,9 +76,18 @@ TargetToken->SpacesRequiredBefore - 1, TargetToken->StartsColumn); }; + const auto IsPPConditional = [&](const size_t LineIndex) { + const auto &Line = Lines[LineIndex]; + return Line->First->is(tok::hash) && Line->First->Next && + Line->First->Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_else, + tok::pp_ifndef, tok::pp_elifndef, + tok::pp_elifdef, tok::pp_elif, + tok::pp_endif); + }; const auto FollowingOtherOpening = [&]() { return OpeningLineIndex == 0 || - Lines[OpeningLineIndex - 1]->Last->opensScope(); + Lines[OpeningLineIndex - 1]->Last->opensScope() || + IsPPConditional(OpeningLineIndex - 1); }; const auto HasEnumOnLine = [CurrentLine]() { FormatToken *CurrentToken = CurrentLine->First; @@ -87,17 +100,29 @@ }; bool IsDefBlock = false; + const auto MayPrecedeDefinition = [&](const int Direction = -1) { + const size_t OperateIndex = OpeningLineIndex + Direction; + assert(OperateIndex < Lines.size()); + const auto &OperateLine = Lines[OperateIndex]; + return (Style.isCSharp() && OperateLine->First->is(TT_AttributeSquare)) || + OperateLine->First->is(tok::comment); + }; if (HasEnumOnLine()) { // We have no scope opening/closing information for enum. IsDefBlock = true; OpeningLineIndex = I; - TargetLine = CurrentLine; - TargetToken = CurrentLine->First; + while (OpeningLineIndex > 0 && MayPrecedeDefinition()) + --OpeningLineIndex; + OpeningLine = Lines[OpeningLineIndex]; + TargetLine = OpeningLine; + TargetToken = TargetLine->First; if (!FollowingOtherOpening()) InsertReplacement(NewlineCount); - else + else if (IsNeverStyle) InsertReplacement(OpeningLineIndex != 0); + TargetLine = CurrentLine; + TargetToken = TargetLine->First; while (TargetToken && !TargetToken->is(tok::r_brace)) TargetToken = TargetToken->Next; if (!TargetToken) { @@ -108,40 +133,48 @@ if (OpeningLineIndex > Lines.size()) continue; // Handling the case that opening bracket has its own line. - OpeningLineIndex -= Lines[OpeningLineIndex]->First->TokenText == "{"; - AnnotatedLine *OpeningLine = Lines[OpeningLineIndex]; + OpeningLineIndex -= Lines[OpeningLineIndex]->First->is(tok::l_brace); + OpeningLine = Lines[OpeningLineIndex]; // Closing a function definition. if (LikelyDefinition(OpeningLine)) { IsDefBlock = true; - if (OpeningLineIndex > 0) { - OpeningLineIndex -= - Style.Language == FormatStyle::LK_CSharp && - Lines[OpeningLineIndex - 1]->First->is(tok::l_square); - OpeningLine = Lines[OpeningLineIndex]; - } + while (OpeningLineIndex > 0 && MayPrecedeDefinition()) + --OpeningLineIndex; + OpeningLine = Lines[OpeningLineIndex]; TargetLine = OpeningLine; TargetToken = TargetLine->First; if (!FollowingOtherOpening()) { // Avoid duplicated replacement. - if (!TargetToken->opensScope()) + if (TargetToken->isNot(tok::l_brace)) InsertReplacement(NewlineCount); - } else + } else if (IsNeverStyle) InsertReplacement(OpeningLineIndex != 0); } } // Not the last token. if (IsDefBlock && I + 1 < Lines.size()) { - TargetLine = Lines[I + 1]; + OpeningLineIndex = I + 1; + TargetLine = Lines[OpeningLineIndex]; TargetToken = TargetLine->First; // No empty line for continuously closing scopes. The token will be // handled in another case if the line following is opening a // definition. - if (!TargetToken->closesScope()) { - if (!LikelyDefinition(TargetLine)) + if (!TargetToken->closesScope() && !IsPPConditional(OpeningLineIndex)) { + // Check whether current line may be precedings of a definition line. + while (OpeningLineIndex + 1 < Lines.size() && + MayPrecedeDefinition(/*Direction=*/0)) + ++OpeningLineIndex; + TargetLine = Lines[OpeningLineIndex]; + if (!LikelyDefinition(TargetLine)) { + TargetLine = Lines[I + 1]; + TargetToken = TargetLine->First; InsertReplacement(NewlineCount); - } else { + } + } else if (IsNeverStyle) { + TargetLine = Lines[I + 1]; + TargetToken = TargetLine->First; InsertReplacement(OpeningLineIndex != 0); } } diff --git a/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp b/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp --- a/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp +++ b/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp @@ -57,8 +57,9 @@ InverseStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always; EXPECT_EQ(ExpectedCode.str(), separateDefinitionBlocks(ExpectedCode, Style)) << "Expected code is not stable"; - std::string InverseResult = separateDefinitionBlocks(Code, InverseStyle); - EXPECT_NE(Code.str(), InverseResult) + std::string InverseResult = + separateDefinitionBlocks(ExpectedCode, InverseStyle); + EXPECT_NE(ExpectedCode.str(), InverseResult) << "Inverse formatting makes no difference"; std::string CodeToFormat = HasOriginalCode ? Code.str() : removeEmptyLines(Code); @@ -129,49 +130,166 @@ Style); } +TEST_F(DefinitionBlockSeparatorTest, UntouchBlockStartStyle) { + // Returns a std::pair of two strings, with the first one for passing into + // Always test and the second one be the expected result of the first string. + auto MakeUntouchTest = [&](std::string BlockHeader, std::string BlockChanger, + std::string BlockFooter, bool BlockEndNewLine) { + std::string CodePart1 = "enum Foo { FOO, BAR };\n" + "\n" + "/*\n" + "test1\n" + "test2\n" + "*/\n" + "int foo(int i, int j) {\n" + " int r = i + j;\n" + " return r;\n" + "}\n"; + std::string CodePart2 = "/* Comment block in one line*/\n" + "enum Bar { FOOBAR, BARFOO };\n" + "\n" + "int bar3(int j, int k) {\n" + " // A comment\n" + " int r = j % k;\n" + " return r;\n" + "}\n"; + std::string CodePart3 = "int bar2(int j, int k) {\n" + " int r = j / k;\n" + " return r;\n" + "}\n"; + std::string ConcatAll = BlockHeader + CodePart1 + BlockChanger + CodePart2 + + BlockFooter + (BlockEndNewLine ? "\n" : "") + + CodePart3; + return std::make_pair(BlockHeader + removeEmptyLines(CodePart1) + + BlockChanger + removeEmptyLines(CodePart2) + + BlockFooter + removeEmptyLines(CodePart3), + ConcatAll); + }; + + FormatStyle AlwaysStyle = getLLVMStyle(); + AlwaysStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always; + + FormatStyle NeverStyle = getLLVMStyle(); + NeverStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Never; + + auto TestKit = MakeUntouchTest("#ifdef FOO\n\n", "\n#elifndef BAR\n\n", + "\n#endif\n\n", false); + verifyFormat(TestKit.first, AlwaysStyle, TestKit.second); + verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second)); + + TestKit = + MakeUntouchTest("#ifdef FOO\n", "#elifndef BAR\n", "#endif\n", false); + verifyFormat(TestKit.first, AlwaysStyle, TestKit.second); + verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second)); + + TestKit = MakeUntouchTest("namespace Ns {\n\n", + "\n} // namespace Ns\n\n" + "namespace {\n\n", + "\n} // namespace\n", true); + verifyFormat(TestKit.first, AlwaysStyle, TestKit.second); + verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second)); + + TestKit = MakeUntouchTest("namespace Ns {\n", + "} // namespace Ns\n\n" + "namespace {\n", + "} // namespace\n", true); + verifyFormat(TestKit.first, AlwaysStyle, TestKit.second); + verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second)); +} + TEST_F(DefinitionBlockSeparatorTest, Always) { FormatStyle Style = getLLVMStyle(); Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always; std::string Prefix = "namespace {\n"; - std::string Postfix = "enum Foo { FOO, BAR };\n" - "\n" - "int foo(int i, int j) {\n" - " int r = i + j;\n" - " return r;\n" - "}\n" + std::string Infix = "\n" + "// Enum test1\n" + "// Enum test2\n" + "enum Foo { FOO, BAR };\n" + "\n" + "/*\n" + "test1\n" + "test2\n" + "*/\n" + "int foo(int i, int j) {\n" + " int r = i + j;\n" + " return r;\n" + "}\n" + "\n" + "// Foobar\n" + "int i, j, k;\n" + "\n" + "// Comment for function\n" + "// Comment line 2\n" + "// Comment line 3\n" + "int bar(int j, int k) {\n" + " int r = j * k;\n" + " return r;\n" + "}\n" + "\n" + "int bar2(int j, int k) {\n" + " int r = j / k;\n" + " return r;\n" + "}\n" + "\n" + "/* Comment block in one line*/\n" + "enum Bar { FOOBAR, BARFOO };\n" + "\n" + "int bar3(int j, int k) {\n" + " // A comment\n" + " int r = j % k;\n" + " return r;\n" + "}\n"; + std::string Postfix = "\n" + "} // namespace\n" "\n" + "namespace T {\n" "int i, j, k;\n" - "\n" - "int bar(int j, int k) {\n" - " int r = j * k;\n" - " return r;\n" - "}\n" - "\n" - "enum Bar { FOOBAR, BARFOO };\n" - "} // namespace"; - verifyFormat(Prefix + "\n\n\n" + removeEmptyLines(Postfix), Style, - Prefix + Postfix); + "} // namespace T"; + verifyFormat(Prefix + removeEmptyLines(Infix) + removeEmptyLines(Postfix), + Style, Prefix + Infix + Postfix); } TEST_F(DefinitionBlockSeparatorTest, Never) { FormatStyle Style = getLLVMStyle(); Style.SeparateDefinitionBlocks = FormatStyle::SDS_Never; std::string Prefix = "namespace {\n"; - std::string Postfix = "enum Foo { FOO, BAR };\n" + std::string Postfix = "// Enum test1\n" + "// Enum test2\n" + "enum Foo { FOO, BAR };\n" "\n" + "/*\n" + "test1\n" + "test2\n" + "*/\n" "int foo(int i, int j) {\n" " int r = i + j;\n" " return r;\n" "}\n" "\n" + "// Foobar\n" "int i, j, k;\n" "\n" + "// Comment for function\n" + "// Comment line 2\n" + "// Comment line 3\n" "int bar(int j, int k) {\n" " int r = j * k;\n" " return r;\n" "}\n" "\n" + "int bar2(int j, int k) {\n" + " int r = j / k;\n" + " return r;\n" + "}\n" + "\n" + "/* Comment block in one line*/\n" "enum Bar { FOOBAR, BARFOO };\n" + "\n" + "int bar3(int j, int k) {\n" + " // A comment\n" + " int r = j % k;\n" + " return r;\n" + "}\n" "} // namespace"; verifyFormat(Prefix + "\n\n\n" + Postfix, Style, Prefix + removeEmptyLines(Postfix)); @@ -181,31 +299,57 @@ FormatStyle Style = getLLVMStyle(); Style.BreakBeforeBraces = FormatStyle::BS_Allman; Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always; - verifyFormat("enum Foo\n" + verifyFormat("namespace NS\n" + "{\n" + "// Enum test1\n" + "// Enum test2\n" + "enum Foo\n" "{\n" " FOO,\n" " BAR\n" "};\n" "\n" + "/*\n" + "test1\n" + "test2\n" + "*/\n" "int foo(int i, int j)\n" "{\n" " int r = i + j;\n" " return r;\n" "}\n" "\n" + "// Foobar\n" "int i, j, k;\n" "\n" + "// Comment for function\n" + "// Comment line 2\n" + "// Comment line 3\n" "int bar(int j, int k)\n" "{\n" " int r = j * k;\n" " return r;\n" "}\n" "\n" + "int bar2(int j, int k)\n" + "{\n" + " int r = j / k;\n" + " return r;\n" + "}\n" + "\n" "enum Bar\n" "{\n" " FOOBAR,\n" " BARFOO\n" - "};", + "};\n" + "\n" + "int bar3(int j, int k)\n" + "{\n" + " // A comment\n" + " int r = j % k;\n" + " return r;\n" + "}\n" + "} // namespace NS", Style); } @@ -215,21 +359,42 @@ Style.MaxEmptyLinesToKeep = 3; std::string LeaveAs = "namespace {\n" "\n" + "// Enum test1\n" + "// Enum test2\n" "enum Foo { FOO, BAR };\n" "\n\n\n" + "/*\n" + "test1\n" + "test2\n" + "*/\n" "int foo(int i, int j) {\n" " int r = i + j;\n" " return r;\n" "}\n" "\n" + "// Foobar\n" "int i, j, k;\n" "\n" + "// Comment for function\n" + "// Comment line 2\n" + "// Comment line 3\n" "int bar(int j, int k) {\n" " int r = j * k;\n" " return r;\n" "}\n" "\n" + "int bar2(int j, int k) {\n" + " int r = j / k;\n" + " return r;\n" + "}\n" + "\n" + "// Comment for inline enum\n" "enum Bar { FOOBAR, BARFOO };\n" + "int bar3(int j, int k) {\n" + " // A comment\n" + " int r = j % k;\n" + " return r;\n" + "}\n" "} // namespace"; verifyFormat(LeaveAs, Style, LeaveAs); } @@ -251,6 +416,7 @@ "internal static String toString() {\r\n" "}\r\n" "\r\n" + "// Comment for enum\r\n" "public enum var {\r\n" " none,\r\n" " @string,\r\n" @@ -258,6 +424,7 @@ " @enum\r\n" "}\r\n" "\r\n" + "// Test\r\n" "[STAThread]\r\n" "static void Main(string[] args) {\r\n" " Console.WriteLine(\"HelloWorld\");\r\n"