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

[Manuel Klimek]

klimek added inline comments.

================
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) {
----------------
djasper wrote:
> 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.
I don't understand yet why we can't use 1 rewriter. The rewriter should automatically use the updated ranges after we did the fixes, shouldn't it?

I definitely think we'll need to have 2 different runs anyway for all integrations that run on replacements, or otherwise we'd need to somehow group replacements in a way that it's clear in which order they need to be applied, and when the basis for the calculation of the ranges changes.

================
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"));
+}
+
----------------
djasper wrote:
> 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.
SG


http://reviews.llvm.org/D11240