[clang] ee25a32 - [clang-format] Fix SeparateDefinitionBlocks issues
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 11 09:25:47 PST 2022
Author: ksyx
Date: 2022-01-11T12:25:39-05:00
New Revision: ee25a327aac0eeae28f468a741b58ec7689de5f2
URL: https://github.com/llvm/llvm-project/commit/ee25a327aac0eeae28f468a741b58ec7689de5f2
DIFF: https://github.com/llvm/llvm-project/commit/ee25a327aac0eeae28f468a741b58ec7689de5f2.diff
LOG: [clang-format] Fix SeparateDefinitionBlocks issues
Fixes https://github.com/llvm/llvm-project/issues/52976.
- Make no formatting for macros
- Attach comment with definition headers
- Make no change on use of empty lines at block start/end
- Fix misrecognition of keyword namespace
Differential Revision: https://reviews.llvm.org/D116663
Reviewed By: MyDeveloperDay, HazardyKnusperkeks, curdeius
Added:
Modified:
clang/lib/Format/DefinitionBlockSeparator.cpp
clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Format/DefinitionBlockSeparator.cpp b/clang/lib/Format/DefinitionBlockSeparator.cpp
index 7f6a7fa1b5dfa..504392cb61422 100644
--- a/clang/lib/Format/DefinitionBlockSeparator.cpp
+++ b/clang/lib/Format/DefinitionBlockSeparator.cpp
@@ -31,15 +31,16 @@ std::pair<tooling::Replacements, unsigned> DefinitionBlockSeparator::analyze(
void DefinitionBlockSeparator::separateBlocks(
SmallVectorImpl<AnnotatedLine *> &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 @@ void DefinitionBlockSeparator::separateBlocks(
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 @@ void DefinitionBlockSeparator::separateBlocks(
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 @@ void DefinitionBlockSeparator::separateBlocks(
};
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 @@ void DefinitionBlockSeparator::separateBlocks(
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
index 91933956c1741..13054bcad1a96 100644
--- a/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
+++ b/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
@@ -57,8 +57,9 @@ class DefinitionBlockSeparatorTest : public ::testing::Test {
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
diff erence";
std::string CodeToFormat =
HasOriginalCode ? Code.str() : removeEmptyLines(Code);
@@ -129,49 +130,166 @@ TEST_F(DefinitionBlockSeparatorTest, Basic) {
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 @@ TEST_F(DefinitionBlockSeparatorTest, OpeningBracketOwnsLine) {
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 @@ TEST_F(DefinitionBlockSeparatorTest, Leave) {
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 @@ TEST_F(DefinitionBlockSeparatorTest, CSharp) {
"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 @@ TEST_F(DefinitionBlockSeparatorTest, CSharp) {
" @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"
More information about the cfe-commits
mailing list