r357599 - [clang-format] Do not emit replacements while regrouping if Cpp includes are OK
Krasimir Georgiev via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 3 08:16:05 PDT 2019
Author: krasimir
Date: Wed Apr 3 08:16:04 2019
New Revision: 357599
URL: http://llvm.org/viewvc/llvm-project?rev=357599&view=rev
Log:
[clang-format] Do not emit replacements while regrouping if Cpp includes are OK
Summary:
Currently clang-format would always emit a replacement for multi-block #include
sections if `IBS_Regroup`, even if the sections are correct:
```
% cat ~/test.h
#include <a.h>
#include "b.h"
% bin/clang-format --output-replacements-xml -style=google ~/test.h
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='0' length='30'>#include <a.h>
#include "b.h"</replacement>
</replacements>
%
```
This change makes clang-format not emit replacements in this case.
The logic is similar to the one implemented for Java in r354452.
Reviewers: ioeric
Reviewed By: ioeric
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D60199
Modified:
cfe/trunk/lib/Format/Format.cpp
cfe/trunk/unittests/Format/SortIncludesTest.cpp
Modified: cfe/trunk/lib/Format/Format.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=357599&r1=357598&r2=357599&view=diff
==============================================================================
--- cfe/trunk/lib/Format/Format.cpp (original)
+++ cfe/trunk/lib/Format/Format.cpp Wed Apr 3 08:16:04 2019
@@ -1753,6 +1753,7 @@ FindCursorIndex(const SmallVectorImpl<In
static void sortCppIncludes(const FormatStyle &Style,
const SmallVectorImpl<IncludeDirective> &Includes,
ArrayRef<tooling::Range> Ranges, StringRef FileName,
+ StringRef Code,
tooling::Replacements &Replaces, unsigned *Cursor) {
unsigned IncludesBeginOffset = Includes.front().Offset;
unsigned IncludesEndOffset =
@@ -1788,6 +1789,10 @@ static void sortCppIncludes(const Format
// If the #includes are out of order, we generate a single replacement fixing
// the entire block. Otherwise, no replacement is generated.
+ // In case Style.IncldueStyle.IncludeBlocks != IBS_Preserve, this check is not
+ // enough as additional newlines might be added or removed across #include
+ // blocks. This we handle below by generating the updated #imclude blocks and
+ // comparing it to the original.
if (Indices.size() == Includes.size() &&
std::is_sorted(Indices.begin(), Indices.end()) &&
Style.IncludeStyle.IncludeBlocks == tooling::IncludeStyle::IBS_Preserve)
@@ -1808,6 +1813,11 @@ static void sortCppIncludes(const Format
CurrentCategory = Includes[Index].Category;
}
+ // 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))
+ return;
+
auto Err = Replaces.add(tooling::Replacement(
FileName, Includes.front().Offset, IncludesBlockSize, result));
// FIXME: better error handling. For now, just skip the replacement for the
@@ -1876,8 +1886,8 @@ tooling::Replacements sortCppIncludes(co
MainIncludeFound = true;
IncludesInBlock.push_back({IncludeName, Line, Prev, Category});
} else if (!IncludesInBlock.empty() && !EmptyLineSkipped) {
- sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces,
- Cursor);
+ sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Code,
+ Replaces, Cursor);
IncludesInBlock.clear();
FirstIncludeBlock = false;
}
@@ -1887,8 +1897,10 @@ tooling::Replacements sortCppIncludes(co
break;
SearchFrom = Pos + 1;
}
- if (!IncludesInBlock.empty())
- sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces, Cursor);
+ if (!IncludesInBlock.empty()) {
+ sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Code, Replaces,
+ Cursor);
+ }
return Replaces;
}
Modified: cfe/trunk/unittests/Format/SortIncludesTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/SortIncludesTest.cpp?rev=357599&r1=357598&r2=357599&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/SortIncludesTest.cpp (original)
+++ cfe/trunk/unittests/Format/SortIncludesTest.cpp Wed Apr 3 08:16:04 2019
@@ -8,6 +8,7 @@
#include "FormatTestUtils.h"
#include "clang/Format/Format.h"
+#include "llvm/ADT/None.h"
#include "llvm/Support/Debug.h"
#include "gtest/gtest.h"
@@ -24,9 +25,11 @@ protected:
}
std::string sort(StringRef Code, std::vector<tooling::Range> Ranges,
- StringRef FileName = "input.cc") {
+ StringRef FileName = "input.cc",
+ unsigned ExpectedNumRanges = 1) {
auto Replaces = sortIncludes(FmtStyle, Code, Ranges, FileName);
Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
+ EXPECT_EQ(ExpectedNumRanges, Replaces.size());
auto Sorted = applyAllReplacements(Code, Replaces);
EXPECT_TRUE(static_cast<bool>(Sorted));
auto Result = applyAllReplacements(
@@ -35,8 +38,10 @@ protected:
return *Result;
}
- std::string sort(StringRef Code, StringRef FileName = "input.cpp") {
- return sort(Code, GetCodeRange(Code), FileName);
+ std::string sort(StringRef Code,
+ StringRef FileName = "input.cpp",
+ unsigned ExpectedNumRanges = 1) {
+ return sort(Code, GetCodeRange(Code), FileName, ExpectedNumRanges);
}
unsigned newCursor(llvm::StringRef Code, unsigned Cursor) {
@@ -151,7 +156,7 @@ TEST_F(SortIncludesTest, SupportClangFor
"#include <b>\n"
"#include <a>\n"
"#include <c>\n"
- "/* clang-format onwards */\n"));
+ "/* clang-format onwards */\n", "input.h", 2));
}
TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
@@ -161,7 +166,8 @@ TEST_F(SortIncludesTest, IncludeSortingC
"#include \"b.h\"\n",
sort("#include \"a.h\"\n"
"#include \"c.h\"\n"
- "#include \"b.h\"\n"));
+ "#include \"b.h\"\n",
+ "input.h", 0));
}
TEST_F(SortIncludesTest, MixIncludeAndImport) {
@@ -214,7 +220,7 @@ TEST_F(SortIncludesTest, SortsLocallyInE
sort("#include \"a.h\"\n"
"#include \"c.h\"\n"
"\n"
- "#include \"b.h\"\n"));
+ "#include \"b.h\"\n", "input.h", 0));
}
TEST_F(SortIncludesTest, SortsAllBlocksWhenMerging) {
@@ -458,7 +464,7 @@ TEST_F(SortIncludesTest, NegativePriorit
sort("#include \"important_os_header.h\"\n"
"#include \"c_main.h\"\n"
"#include \"a_other.h\"\n",
- "c_main.cc"));
+ "c_main.cc", 0));
}
TEST_F(SortIncludesTest, PriorityGroupsAreSeparatedWhenRegroupping) {
@@ -486,7 +492,7 @@ TEST_F(SortIncludesTest, PriorityGroupsA
"#include \"c_main.h\"\n"
"\n"
"#include \"a_other.h\"\n",
- "c_main.cc"));
+ "c_main.cc", 0));
}
TEST_F(SortIncludesTest, CalculatesCorrectCursorPosition) {
@@ -634,7 +640,17 @@ TEST_F(SortIncludesTest, DoNotSortLikely
sort("<!--;\n"
"#include <b>\n"
"#include <a>\n"
- "-->"));
+ "-->", "input.h", 0));
+}
+
+TEST_F(SortIncludesTest, DoNotOutputReplacementsForSortedBlocksWithRegrouping) {
+ Style.IncludeBlocks = Style.IBS_Regroup;
+ std::string Code = R"(
+#include "b.h"
+
+#include <a.h>
+)";
+ EXPECT_EQ(Code, sort(Code, "input.h", 0));
}
} // end namespace
More information about the cfe-commits
mailing list