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