[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK
MyDeveloperDay added a comment. Possible fix for PR43372 (https://bugs.llvm.org/show_bug.cgi?id=43372) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60199/new/ https://reviews.llvm.org/D60199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK
This revision was automatically updated to reflect the committed changes. Closed by commit rC357599: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK (authored by krasimir, committed by ). Changed prior to commit: https://reviews.llvm.org/D60199?vs=193500&id=193508#toc Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60199/new/ https://reviews.llvm.org/D60199 Files: lib/Format/Format.cpp unittests/Format/SortIncludesTest.cpp 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,11 @@ } std::string sort(StringRef Code, std::vector 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(Sorted)); auto Result = applyAllReplacements( @@ -35,8 +38,10 @@ 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 @@ "#include \n" "#include \n" "#include \n" - "/* clang-format onwards */\n")); + "/* clang-format onwards */\n", "input.h", 2)); } TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) { @@ -161,7 +166,8 @@ "#include \"b.h\"\n", sort("#include \"a.h\"\n" "#include \"c.h\"\n" - "#include \"b.h\"\n")); + "#include \"b.h\"\n", +
[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK
krasimir updated this revision to Diff 193500. krasimir added a comment. - Address review comments Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60199/new/ 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,11 @@ } std::string sort(StringRef Code, std::vector 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(Sorted)); auto Result = applyAllReplacements( @@ -35,8 +38,10 @@ 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 @@ "#include \n" "#include \n" "#include \n" - "/* clang-format onwards */\n")); + "/* clang-format onwards */\n", "input.h", 2)); } TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) { @@ -161,7 +166,8 @@ "#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 @@ 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 @@ 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 @@ "#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 @@ sort("")); + "-->", "input.h", 0)); +} + +TEST_F(SortIncludesTest, DoNotOutputReplacementsForSortedBlocksWithRegrouping) { + Style.IncludeBlocks = Style.IBS_Regroup; + std::string Code = R"( +#include "b.h" + +#include +)"; + EXPECT_EQ(Code, sort(Code, "input.h", 0)); } } // end namespace 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, Include
[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK
ioeric added inline comments. Comment at: unittests/Format/SortIncludesTest.cpp:28 std::string sort(StringRef Code, std::vector Ranges, + llvm::Optional ExpectedNumRanges = llvm::None, StringRef FileName = "input.cc") { As most cases would pass `1`, maybe it would make sense to make this a class member that defaults to `1`? It can be explicitly set in specific tests when needed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60199/new/ https://reviews.llvm.org/D60199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK
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 #include "b.h" % bin/clang-format --output-replacements-xml -style=google ~/test.h #include#include "b.h" % 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 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"