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

[Daniel Jasper]

djasper marked 7 inline comments as done.

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

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

================
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 {
----------------
klimek wrote:
> Aren't now the ranges incorrect?
So, there are two cases:
1) Ranges outside of #include blocks. These should not change as the size of the include block doesn't change.
2) Ranges inside of a #include block. Yes, here we kind of format a #include statement that isn't necessarily the one that was included in the original range. I think the right fix here is to format the entire #include block if something is moved around (to re-align comments if necessary). What do you think? Is that ok as a follow-up patch?

================
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) {
----------------
klimek wrote:
> 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 (?)
Yes, using a second rewriter or recomputing the affected ranges is a significant calculation for the sole purpose of un-doing the offsets calculated in the rewriter. Furthermore, we already have the new code after #include sorting, which we simply calculate again, which seems like wasted efforts.

I don't know how the non-atomic-write has been a problem, but if it is, I'd prefer to pull out a function or the AtomicallyMovedFile.

I am strongly against letting this be handled by editor integrations so that the integrations exhibit consistent behavior and that we don't need to implement this N times.

================
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"));
+}
+
----------------
klimek wrote:
> Do we want to sort C vs C++ system headers?
Yes, but I want to do refinements to the actual sorting in a follow up. I have a bunch of ideas.


http://reviews.llvm.org/D11240