[PATCH] D58436: [clang-format] Do not emit replacements if Java imports are OK

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir created this revision.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

Currently clang-format would always emit a replacement for a block of Java imports even if it is correctly formatted:

  % cat /tmp/Aggregator.java
  import X;
  % clang-format /tmp/Aggregator.java
  import X;
  % clang-format -output-replacements-xml /tmp/Aggregator.java
  <?xml version='1.0'?>
  <replacements xml:space='preserve' incomplete_format='false'>
  <replacement offset='0' length='9'>import X;</replacement>
  </replacements>
  %

This change makes clang-format not emit replacements in this case. Note that
there is logic to not emit replacements in this case for C++.


Repository:
  rC Clang

https://reviews.llvm.org/D58436

Files:
  lib/Format/Format.cpp
  unittests/Format/SortImportsTestJava.cpp


Index: unittests/Format/SortImportsTestJava.cpp
===================================================================
--- unittests/Format/SortImportsTestJava.cpp
+++ unittests/Format/SortImportsTestJava.cpp
@@ -262,6 +262,14 @@
                  "import org.a;"));
 }
 
+TEST_F(SortImportsTestJava, NoReplacementsForValidImports) {
+  // Identical #includes have led to a failure with an unstable sort.
+  std::string Code = "import org.a;\n"
+                     "import org.b;\n";
+  EXPECT_TRUE(
+      sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1853,6 +1853,7 @@
 static void sortJavaImports(const FormatStyle &Style,
                             const SmallVectorImpl<JavaImportDirective> &Imports,
                             ArrayRef<tooling::Range> Ranges, StringRef FileName,
+                            StringRef Code,
                             tooling::Replacements &Replaces) {
   unsigned ImportsBeginOffset = Imports.front().Offset;
   unsigned ImportsEndOffset =
@@ -1868,12 +1869,12 @@
         findJavaImportGroup(Style, Imports[i].Identifier));
   }
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-        // Negating IsStatic to push static imports above non-static imports.
-        return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-                               Imports[LHSI].Identifier) <
-               std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-                               Imports[RHSI].Identifier);
-      });
+    // Negating IsStatic to push static imports above non-static imports.
+    return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
+                           Imports[LHSI].Identifier) <
+           std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
+                           Imports[RHSI].Identifier);
+  });
 
   // Deduplicate imports.
   Indices.erase(std::unique(Indices.begin(), Indices.end(),
@@ -1902,6 +1903,11 @@
     CurrentImportGroup = JavaImportGroups[Index];
   }
 
+  // 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))
+    return;
+
   auto Err = Replaces.add(tooling::Replacement(FileName, Imports.front().Offset,
                                                ImportsBlockSize, result));
   // FIXME: better error handling. For now, just skip the replacement for the
@@ -1967,7 +1973,7 @@
     SearchFrom = Pos + 1;
   }
   if (!ImportsInBlock.empty())
-    sortJavaImports(Style, ImportsInBlock, Ranges, FileName, Replaces);
+    sortJavaImports(Style, ImportsInBlock, Ranges, FileName, Code, Replaces);
   return Replaces;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58436.187543.patch
Type: text/x-patch
Size: 3012 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190220/8609d6cb/attachment-0001.bin>