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