[PATCH] D52800: Java import sorting in clang-format

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.


================
Comment at: lib/Format/Format.cpp:1823
+  unsigned LongestMatchLength = 0;
+  for (unsigned i = 0; i < Style.JavaImportGroups.size(); i++) {
+    std::string GroupPrefix = Style.JavaImportGroups[i];
----------------
nit: LLVM Style uses CamelCase for variables, please use `I`.


================
Comment at: lib/Format/Format.cpp:1834
+
+// Sorts and deduplicate a block of includes given by 'Imports' based on
+// JavaImportGroups, then adding the necessary replacement to 'Replaces'.
----------------
nit: Sorts and deduplicates


================
Comment at: lib/Format/Format.cpp:1836
+// JavaImportGroups, then adding the necessary replacement to 'Replaces'.
+// import declarations with the same text will be deduplicated. Between each
+// import group, a newline is inserted, and within each import group, a
----------------
nit: Import declarations...


================
Comment at: unittests/Format/SortImportsTestJava.cpp:99
+                 "import org.a;\n"));
+}
+
----------------
Add a test where there are existing blocks of import statements separated by empty lines. I'd expect that a newly added import gets added to the right already existing block, right?


================
Comment at: unittests/Format/SortImportsTestJava.cpp:136
+  EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
+                                    "import org.a;\n"));
+}
----------------
interested does the newline get taken into account? e.g. what happens with
```
sort("import org.a;\n"
     "import org.b;");
```


Repository:
  rC Clang

https://reviews.llvm.org/D52800