[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:848
     ChromiumStyle.IndentWidth = 4;
+    ChromiumStyle.JavaImportGroups = {
+        "android",
----------------
It would be helpful to link to a style guide reference for this.


================
Comment at: lib/Format/Format.cpp:1837-1838
+// import declarations with the same text will be deduplicated. Between each
+// import group, a newline is inserted, and within each import group, a
+// case-sensitive sort is performed.
+static void sortJavaImports(const FormatStyle &Style,
----------------
It would be helpful to spell out the sort order and add tests with respect to `.` and `*`. ASCII ordering has `* < . < A < a` which seems reasonable.


================
Comment at: lib/Format/Format.cpp:1922
+    StringRef Trimmed = Line.trim();
+    if (Trimmed == "// clang-format off")
+      FormattingOff = true;
----------------
Please add a test for this.


================
Comment at: lib/Format/Format.cpp:1932
+        bool IsStatic = false;
+        if (Static.contains("static")) {
+          IsStatic = true;
----------------
Hm, this would also pick up the `static` in `import a.*; // static`, right? IMO not a big problem, just add a note about it.


================
Comment at: unittests/Format/SortImportsTestJava.cpp:130
+                 "// that do\n"
+                 "// nothing\n",
+                 Ranges));
----------------
I wonder what happens if there are comments between import statements and comment lines after import statements. Consider adding some tests for that.


Repository:
  rC Clang

https://reviews.llvm.org/D52800