krasimir created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
krasimir added a reviewer: ioeric.
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.
Repository:
rC Clang
https://reviews.llvm.org/D60199
Files:
lib/Format/Format.cpp
unittests/Format/SortIncludesTest.cpp
Index: unittests/Format/SortIncludesTest.cpp
===================================================================
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -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,13 @@
}
std::string sort(StringRef Code, std::vector<tooling::Range> Ranges,
+ llvm::Optional<unsigned> ExpectedNumRanges = llvm::None,
StringRef FileName = "input.cc") {
auto Replaces = sortIncludes(FmtStyle, Code, Ranges, FileName);
Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
+ if (ExpectedNumRanges) {
+ EXPECT_EQ(*ExpectedNumRanges, Replaces.size());
+ }
auto Sorted = applyAllReplacements(Code, Replaces);
EXPECT_TRUE(static_cast<bool>(Sorted));
auto Result = applyAllReplacements(
@@ -35,8 +40,10 @@
return *Result;
}
- std::string sort(StringRef Code, StringRef FileName = "input.cpp") {
- return sort(Code, GetCodeRange(Code), FileName);
+ std::string sort(StringRef Code,
+ llvm::Optional<unsigned> ExpectedNumRanges = llvm::None,
+ StringRef FileName = "input.cpp") {
+ return sort(Code, GetCodeRange(Code), ExpectedNumRanges, FileName);
}
unsigned newCursor(llvm::StringRef Code, unsigned Cursor) {
@@ -321,6 +328,7 @@
sort("#include \"llvm/a.h\"\n"
"#include \"c.h\"\n"
"#include \"b.h\"\n",
+ 1,
"a.cc"));
EXPECT_EQ("#include \"llvm/a.h\"\n"
"#include \"b.h\"\n"
@@ -328,6 +336,7 @@
sort("#include \"llvm/a.h\"\n"
"#include \"c.h\"\n"
"#include \"b.h\"\n",
+ 1,
"a_test.cc"));
EXPECT_EQ("#include \"llvm/input.h\"\n"
"#include \"b.h\"\n"
@@ -335,6 +344,7 @@
sort("#include \"llvm/input.h\"\n"
"#include \"c.h\"\n"
"#include \"b.h\"\n",
+ 1,
"input.mm"));
// Don't allow prefixes.
@@ -344,6 +354,7 @@
sort("#include \"llvm/not_a.h\"\n"
"#include \"c.h\"\n"
"#include \"b.h\"\n",
+ 1,
"a.cc"));
// Don't do this for _main and other suffixes.
@@ -353,6 +364,7 @@
sort("#include \"llvm/a.h\"\n"
"#include \"c.h\"\n"
"#include \"b.h\"\n",
+ 1,
"a_main.cc"));
// Don't do this in headers.
@@ -362,6 +374,7 @@
sort("#include \"llvm/a.h\"\n"
"#include \"c.h\"\n"
"#include \"b.h\"\n",
+ 1,
"a.h"));
// Only do this in the first #include block.
@@ -375,6 +388,7 @@
"#include \"llvm/a.h\"\n"
"#include \"c.h\"\n"
"#include \"b.h\"\n",
+ 1,
"a.cc"));
// Only recognize the first #include with a matching basename as main include.
@@ -386,6 +400,7 @@
"#include \"a.h\"\n"
"#include \"c.h\"\n"
"#include \"llvm/a.h\"\n",
+ 1,
"a.cc"));
}
@@ -400,6 +415,7 @@
"\n"
"#include \"a.h\"\n"
"#include \"c.h\"\n",
+ 1,
"c.cc"));
}
@@ -415,6 +431,7 @@
"\n"
"#include \"a.h\"\n"
"#include \"c.h\"\n",
+ 1,
"a.cc"));
}
@@ -438,6 +455,7 @@
"#include \"gmock/gmock.h\"\n"
"#include \"llvm/X.h\"\n"
"#include \"LLVM/z.h\"\n",
+ 1,
"a_TEST.cc"));
}
@@ -449,6 +467,7 @@
sort("#include \"c_main.h\"\n"
"#include \"a_other.h\"\n"
"#include \"important_os_header.h\"\n",
+ 1,
"c_main.cc"));
// check stable when re-run
@@ -458,6 +477,7 @@
sort("#include \"important_os_header.h\"\n"
"#include \"c_main.h\"\n"
"#include \"a_other.h\"\n",
+ 0,
"c_main.cc"));
}
@@ -473,6 +493,7 @@
sort("#include \"c_main.h\"\n"
"#include \"a_other.h\"\n"
"#include \"important_os_header.h\"\n",
+ 1,
"c_main.cc"));
// check stable when re-run
@@ -486,6 +507,7 @@
"#include \"c_main.h\"\n"
"\n"
"#include \"a_other.h\"\n",
+ 0,
"c_main.cc"));
}
@@ -637,6 +659,16 @@
"-->"));
}
+TEST_F(SortIncludesTest, DoNotOutputReplacementsForSortedBlocksWithRegrouping) {
+ Style.IncludeBlocks = Style.IBS_Regroup;
+ std::string Code = R"(
+#include "b.h"
+
+#include <a.h>
+)";
+ EXPECT_EQ(Code, sort(Code, 0));
+}
+
} // end namespace
} // end namespace format
} // end namespace clang
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1753,6 +1753,7 @@
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 @@
// 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 @@
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 @@
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 @@
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
