r373388 - [clang-format] [PR43372] - clang-format shows replacements in DOS files when no replacement is needed

[Paul Hoad via cfe-commits]

Author: paulhoad
Date: Tue Oct  1 13:20:22 2019
New Revision: 373388

URL: http://llvm.org/viewvc/llvm-project?rev=373388&view=rev
Log:
[clang-format] [PR43372] - clang-format shows replacements in DOS files when no replacement is needed

Summary:
This is a patch to fix PR43372 (https://bugs.llvm.org/show_bug.cgi?id=43372) - clang-format can't format file with includes, ( which really keep providing replacements for already sorted headers.)

A similar issue was addressed by @krasimir in {D60199}, however, this seemingly only prevented the issue when the files being formatted did not contain windows line endings (\r\n)

It's possible this is related to https://twitter.com/StephanTLavavej/status/1176722938243895296 given who @STL_MSFT  works for!

As people often used the existence of replacements to determine if a file needs clang-formatting, this is probably pretty important for windows users

There may be a better way of comparing 2 strings and ignoring \r (which appear in both Results and Code), I couldn't choose between this idiom or the copy_if approach, but I'm happy to change it to whatever people consider more performant.

Reviewers: krasimir, klimek, owenpan, ioeric

Reviewed By: krasimir

Subscribers: cfe-commits, STL_MSFT, krasimir

Tags: #clang-format, #clang, #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D68227

Modified:
    cfe/trunk/lib/Format/Format.cpp
    cfe/trunk/unittests/Format/SortImportsTestJava.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=373388&r1=373387&r2=373388&view=diff
==============================================================================
--- cfe/trunk/lib/Format/Format.cpp (original)
+++ cfe/trunk/lib/Format/Format.cpp Tue Oct  1 13:20:22 2019
@@ -1825,6 +1825,28 @@ FindCursorIndex(const SmallVectorImpl<In
   return std::make_pair(CursorIndex, OffsetToEOL);
 }
 
+// Replace all "\r\n" with "\n".
+std::string replaceCRLF(const std::string &Code) {
+  std::string NewCode;
+  size_t Pos = 0, LastPos = 0;
+
+  do {
+    Pos = Code.find("\r\n", LastPos);
+    if (Pos == LastPos) {
+      LastPos++;
+      continue;
+    }
+    if (Pos == std::string::npos) {
+      NewCode += Code.substr(LastPos);
+      break;
+    }
+    NewCode += Code.substr(LastPos, Pos - LastPos) + "\n";
+    LastPos = Pos + 2;
+  } while (Pos != std::string::npos);
+
+  return NewCode;
+}
+
 // Sorts and deduplicate a block of includes given by 'Includes' alphabetically
 // adding the necessary replacement to 'Replaces'. 'Includes' must be in strict
 // source order.
@@ -1898,7 +1920,8 @@ static void sortCppIncludes(const Format
 
   // 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))
+  if (replaceCRLF(result) ==
+      replaceCRLF(Code.substr(IncludesBeginOffset, IncludesBlockSize)))
     return;
 
   auto Err = Replaces.add(tooling::Replacement(
@@ -2066,7 +2089,8 @@ static void sortJavaImports(const Format
 
   // If the imports are out of order, we generate a single replacement fixing
   // the entire block. Otherwise, no replacement is generated.
-  if (result == Code.substr(Imports.front().Offset, ImportsBlockSize))
+  if (replaceCRLF(result) ==
+      replaceCRLF(Code.substr(Imports.front().Offset, ImportsBlockSize)))
     return;
 
   auto Err = Replaces.add(tooling::Replacement(FileName, Imports.front().Offset,
@@ -2356,7 +2380,7 @@ reformat(const FormatStyle &Style, Strin
 
   auto Env =
       std::make_unique<Environment>(Code, FileName, Ranges, FirstStartColumn,
-                                     NextStartColumn, LastStartColumn);
+                                    NextStartColumn, LastStartColumn);
   llvm::Optional<std::string> CurrentCode = None;
   tooling::Replacements Fixes;
   unsigned Penalty = 0;

Modified: cfe/trunk/unittests/Format/SortImportsTestJava.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/SortImportsTestJava.cpp?rev=373388&r1=373387&r2=373388&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/SortImportsTestJava.cpp (original)
+++ cfe/trunk/unittests/Format/SortImportsTestJava.cpp Tue Oct  1 13:20:22 2019
@@ -285,6 +285,13 @@ TEST_F(SortImportsTestJava, NoReplacemen
       sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
 }
 
+TEST_F(SortImportsTestJava, NoReplacementsForValidImportsWindows) {
+  std::string Code = "import org.a;\r\n"
+                     "import org.b;\r\n";
+  EXPECT_TRUE(
+      sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang

Modified: cfe/trunk/unittests/Format/SortIncludesTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/SortIncludesTest.cpp?rev=373388&r1=373387&r2=373388&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/SortIncludesTest.cpp (original)
+++ cfe/trunk/unittests/Format/SortIncludesTest.cpp Tue Oct  1 13:20:22 2019
@@ -724,6 +724,14 @@ TEST_F(SortIncludesTest, DoNotOutputRepl
   EXPECT_EQ(Code, sort(Code, "input.h", 0));
 }
 
+TEST_F(SortIncludesTest,
+       DoNotOutputReplacementsForSortedBlocksWithRegroupingWindows) {
+  Style.IncludeBlocks = Style.IBS_Regroup;
+  std::string Code = "#include \"b.h\"\r\n"
+                     "\r\n"
+                     "#include <a.h>\r\n";
+  EXPECT_EQ(Code, sort(Code, "input.h", 0));
+}
 
 TEST_F(SortIncludesTest, DoNotRegroupGroupsInGoogleObjCStyle) {
   FmtStyle = getGoogleStyle(FormatStyle::LK_ObjC);