[PATCH] D11240: Add basic #include sorting functionality to clang-format

[Manuel Klimek]

klimek added inline comments.

================
Comment at: lib/Format/Format.cpp:1582-1586
@@ +1581,7 @@
+
+// Sorts a block of includes given by 'Includes' alphabetically adding the
+// necessary replacement to 'Replaces'.
+static void sortIncludes(const SmallVectorImpl<IncludeDirective> &Includes,
+                         ArrayRef<tooling::Range> Ranges, StringRef FileName,
+                         tooling::Replacements &Replaces) {
+  if (Includes.empty() || 
----------------
Perhaps document that Includes needs to be in strict source order.

================
Comment at: lib/Format/Format.cpp:1612
@@ +1611,3 @@
+  unsigned SearchFrom = 0;
+  llvm::Regex IncludeRegex("#include.*[\"<](.*)([\">])");
+  SmallVector<StringRef, 4> Matches;
----------------
You'll probably want to allow more spaces if this is run before formatting:
\s*#\s*include\s+
Also, we'll probably want .*? in the middle (or [^">]) if there is a > or " in a comment afterwards.

================
Comment at: lib/Format/Format.cpp:1620-1622
@@ +1619,5 @@
+    if (!Line.endswith("\\")) {
+      if (!IncludeRegex.match(Line, &Matches)) {
+        sortIncludes(IncludesInBlock, Ranges, FileName, Replaces);
+        IncludesInBlock.clear();
+      } else {
----------------
Having the ! case first without early exit confused me.

================
Comment at: lib/Format/Format.cpp:1630-1632
@@ +1629,5 @@
+        }
+        IncludesInBlock.push_back({Matches[1], Line, Prev,
+                                   static_cast<unsigned>(Pos - Prev),
+                                   Matches[2] == ">"});
+
----------------
Why does this work if Pos is npos here? Or can that not happen?

================
Comment at: lib/Format/Format.cpp:1635
@@ +1634,3 @@
+      }
+      Prev = Pos + 1;
+    }
----------------
Here I can see that npos doens't matter, because we'll not re-use Prev.

================
Comment at: tools/clang-format/ClangFormat.cpp:217
@@ -204,1 +216,3 @@
 
+static void printReplacements(const tooling::Replacements &Replaces) {
+  for (const auto &R : Replaces) {
----------------
Call this outputReplacementsXML (so it's consistent with outputReplacementXML), or rename all.

================
Comment at: tools/clang-format/ClangFormat.cpp:243-248
@@ +242,8 @@
+  std::string ChangedCode;
+  if (SortIncludes) {
+    StringRef CodeRef = Code.get()->getBuffer();
+    IncludeReplacements =
+        sortIncludes(Code.get()->getBuffer(), Ranges, FileName);
+    ChangedCode =
+        tooling::applyAllReplacements(CodeRef, IncludeReplacements);
+  } else {
----------------
Aren't now the ranges incorrect?

================
Comment at: tools/clang-format/ClangFormat.cpp:277
@@ +276,3 @@
+        std::error_code EC;
+        raw_fd_ostream FileOut(FileName, EC, llvm::sys::fs::F_Text);
+        if (EC) {
----------------
This is not atomic, which has been a problem in the past (and which is why the rewriter's overwriteChangedFiles is carefully crafted).
Any reason to not use a second rewriter? Or even better, can we:
1. Create a rewriter
2. Get affected ranges
3. Iff include sorting, sort includes
4. Recompute affected ranges in new file
5. clang-format
6. Rewrite
Alternatively, do not support include sorting and clang-format in the same run and have editor integrations do 2 calls (?)

================
Comment at: unittests/Format/SortIncludesTest.cpp:31-36
@@ +30,8 @@
+TEST_F(SortIncludesTest, RemovesTrailingWhitespaceOfFormattedLine) {
+  EXPECT_EQ("#include \"aaaa.h\"\n"
+            "#include \"bbbb.h\"\n"
+            "#include \"cccc.h\"\n",
+            sort("#include \"aaaa.h\"\n"
+                 "#include \"cccc.h\"\n"
+                 "#include \"bbbb.h\"\n"));
+}
----------------
Nit: I find it slightly distracting that we're not calling them a.h, b.h, c.h (and perhaps have one test that checks that they support longer filenames).

================
Comment at: unittests/Format/SortIncludesTest.cpp:50-59
@@ +49,12 @@
+
+TEST_F(SortIncludesTest, HandlesAngledIncludesAsSeparateBlocks) {
+  EXPECT_EQ("#include <bbbb.h>\n"
+            "#include <dddd.h>\n"
+            "#include \"aaaa.h\"\n"
+            "#include \"cccc.h\"\n",
+            sort("#include <dddd.h>\n"
+                 "#include <bbbb.h>\n"
+                 "#include \"cccc.h\"\n"
+                 "#include \"aaaa.h\"\n"));
+}
+
----------------
Do we want to sort C vs C++ system headers?


http://reviews.llvm.org/D11240