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 &Includes, ArrayRef 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; } 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 Ranges, + llvm::Optional 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(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 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 +)"; + EXPECT_EQ(Code, sort(Code, 0)); +} + } // end namespace } // end namespace format } // end namespace clang