ksyx updated this revision to Diff 400821. ksyx marked 3 inline comments as done. ksyx edited the summary of this revision. ksyx added a comment.
- Use function keyword for JavaScript instead of comparing strings - Apply review suggestions CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117520/new/ https://reviews.llvm.org/D117520 Files: clang/lib/Format/DefinitionBlockSeparator.cpp clang/lib/Format/DefinitionBlockSeparator.h clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
Index: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp =================================================================== --- clang/unittests/Format/DefinitionBlockSeparatorTest.cpp +++ clang/unittests/Format/DefinitionBlockSeparatorTest.cpp @@ -130,6 +130,41 @@ Style); } +TEST_F(DefinitionBlockSeparatorTest, CommentBlock) { + FormatStyle Style = getLLVMStyle(); + Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always; + std::string Prefix = "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 Suffix = "enum Bar { FOOBAR, BARFOO };\n" + "\n" + "/* Comment block in one line*/\n" + "int bar3(int j, int k) {\n" + " // A comment\n" + " int r = j % k;\n" + " return r;\n" + "}\n"; + std::string CommentedCode = "/*\n" + "int bar2(int j, int k) {\n" + " int r = j / k;\n" + " return r;\n" + "}\n" + "*/\n"; + verifyFormat(removeEmptyLines(Prefix) + "\n" + CommentedCode + "\n" + + removeEmptyLines(Suffix), + Style, Prefix + "\n" + CommentedCode + "\n" + Suffix); + verifyFormat(removeEmptyLines(Prefix) + "\n" + CommentedCode + + removeEmptyLines(Suffix), + Style, Prefix + "\n" + CommentedCode + Suffix); +} + 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. @@ -172,13 +207,15 @@ 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); + auto TestKit = MakeUntouchTest("/* FOOBAR */\n" + "#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); + TestKit = MakeUntouchTest("/* FOOBAR */\n" + "#ifdef FOO\n", + "#elifndef BAR\n", "#endif\n", false); verifyFormat(TestKit.first, AlwaysStyle, TestKit.second); verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second)); @@ -210,7 +247,7 @@ "test1\n" "test2\n" "*/\n" - "int foo(int i, int j) {\n" + "/*const*/ int foo(int i, int j) {\n" " int r = i + j;\n" " return r;\n" "}\n" @@ -222,8 +259,10 @@ "// Comment line 2\n" "// Comment line 3\n" "int bar(int j, int k) {\n" - " int r = j * k;\n" - " return r;\n" + " {\n" + " int r = j * k;\n" + " return r;\n" + " }\n" "}\n" "\n" "int bar2(int j, int k) {\n" @@ -234,7 +273,7 @@ "/* Comment block in one line*/\n" "enum Bar { FOOBAR, BARFOO };\n" "\n" - "int bar3(int j, int k) {\n" + "int bar3(int j, int k, const enum Bar b) {\n" " // A comment\n" " int r = j % k;\n" " return r;\n" @@ -261,7 +300,7 @@ "test1\n" "test2\n" "*/\n" - "int foo(int i, int j) {\n" + "/*const*/ int foo(int i, int j) {\n" " int r = i + j;\n" " return r;\n" "}\n" @@ -273,8 +312,10 @@ "// Comment line 2\n" "// Comment line 3\n" "int bar(int j, int k) {\n" - " int r = j * k;\n" - " return r;\n" + " {\n" + " int r = j * k;\n" + " return r;\n" + " }\n" "}\n" "\n" "int bar2(int j, int k) {\n" @@ -285,7 +326,7 @@ "/* Comment block in one line*/\n" "enum Bar { FOOBAR, BARFOO };\n" "\n" - "int bar3(int j, int k) {\n" + "int bar3(int j, int k, const enum Bar b) {\n" " // A comment\n" " int r = j % k;\n" " return r;\n" @@ -313,7 +354,7 @@ "test1\n" "test2\n" "*/\n" - "int foo(int i, int j)\n" + "/*const*/ int foo(int i, int j)\n" "{\n" " int r = i + j;\n" " return r;\n" @@ -327,8 +368,10 @@ "// Comment line 3\n" "int bar(int j, int k)\n" "{\n" - " int r = j * k;\n" - " return r;\n" + " {\n" + " int r = j * k;\n" + " return r;\n" + " }\n" "}\n" "\n" "int bar2(int j, int k)\n" @@ -343,7 +386,7 @@ " BARFOO\n" "};\n" "\n" - "int bar3(int j, int k)\n" + "int bar3(int j, int k, const enum Bar b)\n" "{\n" " // A comment\n" " int r = j % k;\n" @@ -367,7 +410,7 @@ "test1\n" "test2\n" "*/\n" - "int foo(int i, int j) {\n" + "/*const*/ int foo(int i, int j) {\n" " int r = i + j;\n" " return r;\n" "}\n" @@ -379,8 +422,10 @@ "// Comment line 2\n" "// Comment line 3\n" "int bar(int j, int k) {\n" - " int r = j * k;\n" - " return r;\n" + " {\n" + " int r = j * k;\n" + " return r;\n" + " }\n" "}\n" "\n" "int bar2(int j, int k) {\n" @@ -390,7 +435,7 @@ "\n" "// Comment for inline enum\n" "enum Bar { FOOBAR, BARFOO };\n" - "int bar3(int j, int k) {\n" + "int bar3(int j, int k, const enum Bar b) {\n" " // A comment\n" " int r = j % k;\n" " return r;\n" Index: clang/lib/Format/DefinitionBlockSeparator.h =================================================================== --- clang/lib/Format/DefinitionBlockSeparator.h +++ clang/lib/Format/DefinitionBlockSeparator.h @@ -33,7 +33,7 @@ private: void separateBlocks(SmallVectorImpl<AnnotatedLine *> &Lines, - tooling::Replacements &Result); + tooling::Replacements &Result, FormatTokenLexer &Tokens); }; } // namespace format } // namespace clang Index: clang/lib/Format/DefinitionBlockSeparator.cpp =================================================================== --- clang/lib/Format/DefinitionBlockSeparator.cpp +++ clang/lib/Format/DefinitionBlockSeparator.cpp @@ -25,22 +25,27 @@ assert(Style.SeparateDefinitionBlocks != FormatStyle::SDS_Leave); AffectedRangeMgr.computeAffectedLines(AnnotatedLines); tooling::Replacements Result; - separateBlocks(AnnotatedLines, Result); + separateBlocks(AnnotatedLines, Result, Tokens); return {Result, 0}; } void DefinitionBlockSeparator::separateBlocks( - SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result) { + SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result, + FormatTokenLexer &Tokens) { const bool IsNeverStyle = Style.SeparateDefinitionBlocks == FormatStyle::SDS_Never; - auto LikelyDefinition = [this](const AnnotatedLine *Line) { + const AdditionalKeywords &ExtraKeywords = Tokens.getKeywords(); + auto LikelyDefinition = [this, ExtraKeywords](const AnnotatedLine *Line, + bool ExcludeEnum = false) { 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_enum) || - (Style.isJavaScript() && CurrentToken->TokenText == "function")) + if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct) || + (Style.isJavaScript() && CurrentToken->is(ExtraKeywords.kw_function))) + return true; + if (!ExcludeEnum && CurrentToken->is(tok::kw_enum)) return true; CurrentToken = CurrentToken->Next; } @@ -73,8 +78,8 @@ if (!TargetLine->Affected) return; Whitespaces.replaceWhitespace(*TargetToken, NewlineToInsert, - TargetToken->SpacesRequiredBefore - 1, - TargetToken->StartsColumn); + TargetToken->OriginalColumn, + TargetToken->OriginalColumn); }; const auto IsPPConditional = [&](const size_t LineIndex) { const auto &Line = Lines[LineIndex]; @@ -89,14 +94,18 @@ Lines[OpeningLineIndex - 1]->Last->opensScope() || IsPPConditional(OpeningLineIndex - 1); }; - const auto HasEnumOnLine = [CurrentLine]() { + const auto HasEnumOnLine = [&]() { FormatToken *CurrentToken = CurrentLine->First; + bool FoundEnumKeyword = false; while (CurrentToken) { if (CurrentToken->is(tok::kw_enum)) + FoundEnumKeyword = true; + else if (FoundEnumKeyword && CurrentToken->is(tok::l_brace)) return true; CurrentToken = CurrentToken->Next; } - return false; + return FoundEnumKeyword && I + 1 < Lines.size() && + Lines[I + 1]->First->is(tok::l_brace); }; bool IsDefBlock = false; @@ -104,11 +113,14 @@ 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); + return ((Style.isCSharp() && + OperateLine->First->is(TT_AttributeSquare)) || + OperateLine->First->is(tok::comment)) && + !LikelyDefinition(OperateLine); }; - if (HasEnumOnLine()) { + if (HasEnumOnLine() && + !LikelyDefinition(CurrentLine, /*ExcludeEnum=*/true)) { // We have no scope opening/closing information for enum. IsDefBlock = true; OpeningLineIndex = I; @@ -132,8 +144,13 @@ } else if (CurrentLine->First->closesScope()) { if (OpeningLineIndex > Lines.size()) continue; - // Handling the case that opening bracket has its own line. - OpeningLineIndex -= Lines[OpeningLineIndex]->First->is(tok::l_brace); + // Handling the case that opening brace has its own line, with checking + // whether the last line already had an opening brace to guard against + // misrecognition. + if (OpeningLineIndex > 0 && + Lines[OpeningLineIndex]->First->is(tok::l_brace) && + Lines[OpeningLineIndex - 1]->Last->isNot(tok::l_brace)) + --OpeningLineIndex; OpeningLine = Lines[OpeningLineIndex]; // Closing a function definition. if (LikelyDefinition(OpeningLine)) {
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits