MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: krasimir, klimek, owenpan, ioeric.
MyDeveloperDay added a project: clang-format.
Herald added a project: clang.
MyDeveloperDay edited the summary of this revision.
This is a patch to fix PR43372 (https://bugs.llvm.org/show_bug.cgi?id=43372) -
clang-format can't format file with includes, ( which really keep providing
replacements for already sorted headers.)
A similar issue was addressed by @krasimir in D60199: [clang-format] Do not
emit replacements while regrouping if Cpp includes are OK
<https://reviews.llvm.org/D60199>, however, this seemingly only prevented the
issue when the files being formatted did not contain windows line endings (\r\n)
It's possible this is related to
https://twitter.com/StephanTLavavej/status/1176722938243895296 given who
@STL_MSFT works for!
As people often used the existence of replacements to determine if a file needs
clang-formatting, this is probably pretty important for windows users
There may be a better way of comparing 2 strings and ignoring \r (which appear
in both Results and Code), I couldn't choose between this idiom or the copy_if
approach, but I'm happy to change it to whatever people consider more
performant.
Repository:
rC Clang
https://reviews.llvm.org/D68227
Files:
clang/lib/Format/Format.cpp
clang/unittests/Format/SortImportsTestJava.cpp
clang/unittests/Format/SortIncludesTest.cpp
Index: clang/unittests/Format/SortIncludesTest.cpp
===================================================================
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -653,6 +653,14 @@
EXPECT_EQ(Code, sort(Code, "input.h", 0));
}
+TEST_F(SortIncludesTest,
+ DoNotOutputReplacementsForSortedBlocksWithRegroupingWindows) {
+ Style.IncludeBlocks = Style.IBS_Regroup;
+ std::string Code = "#include \"b.h\"\r\n"
+ "\r\n"
+ "#include <a.h>\r\n";
+ EXPECT_EQ(Code, sort(Code, "input.h", 0));
+}
TEST_F(SortIncludesTest, DoNotRegroupGroupsInGoogleObjCStyle) {
FmtStyle = getGoogleStyle(FormatStyle::LK_ObjC);
Index: clang/unittests/Format/SortImportsTestJava.cpp
===================================================================
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -285,6 +285,13 @@
sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
}
+TEST_F(SortImportsTestJava, NoReplacementsForValidImportsWindows) {
+ std::string Code = "import org.a;\r\n"
+ "import org.b;\r\n";
+ EXPECT_TRUE(
+ sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
+}
+
} // end namespace
} // end namespace format
} // end namespace clang
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1808,6 +1808,14 @@
return std::make_pair(CursorIndex, OffsetToEOL);
}
+// Compares two strings but ignores any \r in either strings.
+static bool compareIgnoringCarriageReturns(std::string Result,
+ std::string Code) {
+ Result.erase(std::remove(Result.begin(), Result.end(), '\r'), Result.end());
+ Code.erase(std::remove(Code.begin(), Code.end(), '\r'), Code.end());
+ return Result == Code;
+}
+
// Sorts and deduplicate a block of includes given by 'Includes' alphabetically
// adding the necessary replacement to 'Replaces'. 'Includes' must be in strict
// source order.
@@ -1879,7 +1887,8 @@
// If the #includes are out of order, we generate a single replacement fixing
// the entire range of blocks. Otherwise, no replacement is generated.
- if (result == Code.substr(IncludesBeginOffset, IncludesBlockSize))
+ if (compareIgnoringCarriageReturns(
+ result, Code.substr(IncludesBeginOffset, IncludesBlockSize)))
return;
auto Err = Replaces.add(tooling::Replacement(
@@ -2044,7 +2053,8 @@
// If the imports are out of order, we generate a single replacement fixing
// the entire block. Otherwise, no replacement is generated.
- if (result == Code.substr(Imports.front().Offset, ImportsBlockSize))
+ if (compareIgnoringCarriageReturns(
+ result, Code.substr(Imports.front().Offset, ImportsBlockSize)))
return;
auto Err = Replaces.add(tooling::Replacement(FileName,
Imports.front().Offset,
@@ -2334,7 +2344,7 @@
auto Env =
std::make_unique<Environment>(Code, FileName, Ranges, FirstStartColumn,
- NextStartColumn, LastStartColumn);
+ NextStartColumn, LastStartColumn);
llvm::Optional<std::string> CurrentCode = None;
tooling::Replacements Fixes;
unsigned Penalty = 0;
Index: clang/unittests/Format/SortIncludesTest.cpp
===================================================================
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -653,6 +653,14 @@
EXPECT_EQ(Code, sort(Code, "input.h", 0));
}
+TEST_F(SortIncludesTest,
+ DoNotOutputReplacementsForSortedBlocksWithRegroupingWindows) {
+ Style.IncludeBlocks = Style.IBS_Regroup;
+ std::string Code = "#include \"b.h\"\r\n"
+ "\r\n"
+ "#include <a.h>\r\n";
+ EXPECT_EQ(Code, sort(Code, "input.h", 0));
+}
TEST_F(SortIncludesTest, DoNotRegroupGroupsInGoogleObjCStyle) {
FmtStyle = getGoogleStyle(FormatStyle::LK_ObjC);
Index: clang/unittests/Format/SortImportsTestJava.cpp
===================================================================
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -285,6 +285,13 @@
sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
}
+TEST_F(SortImportsTestJava, NoReplacementsForValidImportsWindows) {
+ std::string Code = "import org.a;\r\n"
+ "import org.b;\r\n";
+ EXPECT_TRUE(
+ sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
+}
+
} // end namespace
} // end namespace format
} // end namespace clang
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1808,6 +1808,14 @@
return std::make_pair(CursorIndex, OffsetToEOL);
}
+// Compares two strings but ignores any \r in either strings.
+static bool compareIgnoringCarriageReturns(std::string Result,
+ std::string Code) {
+ Result.erase(std::remove(Result.begin(), Result.end(), '\r'), Result.end());
+ Code.erase(std::remove(Code.begin(), Code.end(), '\r'), Code.end());
+ return Result == Code;
+}
+
// Sorts and deduplicate a block of includes given by 'Includes' alphabetically
// adding the necessary replacement to 'Replaces'. 'Includes' must be in strict
// source order.
@@ -1879,7 +1887,8 @@
// If the #includes are out of order, we generate a single replacement fixing
// the entire range of blocks. Otherwise, no replacement is generated.
- if (result == Code.substr(IncludesBeginOffset, IncludesBlockSize))
+ if (compareIgnoringCarriageReturns(
+ result, Code.substr(IncludesBeginOffset, IncludesBlockSize)))
return;
auto Err = Replaces.add(tooling::Replacement(
@@ -2044,7 +2053,8 @@
// If the imports are out of order, we generate a single replacement fixing
// the entire block. Otherwise, no replacement is generated.
- if (result == Code.substr(Imports.front().Offset, ImportsBlockSize))
+ if (compareIgnoringCarriageReturns(
+ result, Code.substr(Imports.front().Offset, ImportsBlockSize)))
return;
auto Err = Replaces.add(tooling::Replacement(FileName, Imports.front().Offset,
@@ -2334,7 +2344,7 @@
auto Env =
std::make_unique<Environment>(Code, FileName, Ranges, FirstStartColumn,
- NextStartColumn, LastStartColumn);
+ NextStartColumn, LastStartColumn);
llvm::Optional<std::string> CurrentCode = None;
tooling::Replacements Fixes;
unsigned Penalty = 0;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
