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 _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits