[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK

[MyDeveloperDay via Phabricator via cfe-commits]

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

[Krasimir Georgiev via Phabricator via cfe-commits]

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=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 ,
 const SmallVectorImpl ,
 ArrayRef Ranges, StringRef FileName,
+StringRef Code,
 tooling::Replacements , 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 Georgiev via Phabricator via cfe-commits]

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 ,
 const SmallVectorImpl ,
 ArrayRef Ranges, StringRef FileName,
+StringRef Code,
 tooling::Replacements , 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))
+

[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK

[Eric Liu via Phabricator via cfe-commits]

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 Georgiev via Phabricator via cfe-commits]

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 a.h>#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"