Index: clang-tools-extra/trunk/include-fixer/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/include-fixer/CMakeLists.txt +++ clang-tools-extra/trunk/include-fixer/CMakeLists.txt @@ -11,6 +11,7 @@ LINK_LIBS clangAST clangBasic + clangFormat clangFrontend clangLex clangParse Index: clang-tools-extra/trunk/include-fixer/IncludeFixer.h =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixer.h +++ clang-tools-extra/trunk/include-fixer/IncludeFixer.h @@ -29,11 +29,13 @@ public: /// \param SymbolIndexMgr A source for matching symbols to header files. /// \param Replacements Storage for the output of the fixer. + /// \param StyleName Fallback style for reformatting. /// \param MinimizeIncludePaths whether inserted include paths are optimized. IncludeFixerActionFactory( - SymbolIndexManager &SymbolIndexMgr, + SymbolIndexManager &SymbolIndexMgr, std::set &Headers, std::vector &Replacements, - bool MinimizeIncludePaths = true); + StringRef StyleName, bool MinimizeIncludePaths = true); + ~IncludeFixerActionFactory() override; bool @@ -46,11 +48,18 @@ /// The client to use to find cross-references. SymbolIndexManager &SymbolIndexMgr; + /// Headers to be added. + std::set &Headers; + /// Replacements are written here. std::vector &Replacements; /// Whether inserted include paths should be optimized. bool MinimizeIncludePaths; + + /// The fallback format style for formatting after insertion if no + /// clang-format config file was found. + std::string FallbackStyle; }; } // namespace include_fixer Index: clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp +++ clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp @@ -8,6 +8,7 @@ //===----------------------------------------------------------------------===// #include "IncludeFixer.h" +#include "clang/Format/Format.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/Preprocessor.h" @@ -58,8 +59,9 @@ class Action : public clang::ASTFrontendAction, public clang::ExternalSemaSource { public: - explicit Action(SymbolIndexManager &SymbolIndexMgr, bool MinimizeIncludePaths) - : SymbolIndexMgr(SymbolIndexMgr), + explicit Action(SymbolIndexManager &SymbolIndexMgr, StringRef StyleName, + bool MinimizeIncludePaths) + : SymbolIndexMgr(SymbolIndexMgr), FallbackStyle(StyleName), MinimizeIncludePaths(MinimizeIncludePaths) {} std::unique_ptr @@ -234,26 +236,61 @@ return IsSystem ? '<' + Suggestion + '>' : '"' + Suggestion + '"'; } + /// Insert all headers before the first #include in \p Code and run + /// clang-format to sort all headers. + /// \return Replacements for inserting and sorting headers. + std::vector + CreateReplacementsForHeaders(StringRef Code, + const std::set &Headers) { + std::set ExistingHeaders; + + // Create replacements for new headers. + clang::tooling::Replacements Insertions; + if (FirstIncludeOffset == -1U) { + // FIXME: skip header guards. + FirstIncludeOffset = 0; + // If there is no existing #include, then insert an empty line after new + // header block. + if (Code.front() != '\n') + Insertions.insert( + clang::tooling::Replacement(Filename, FirstIncludeOffset, 0, "\n")); + } + // Keep inserting new headers before the first header. + for (StringRef Header : Headers) { + std::string Text = "#include " + Header.str() + "\n"; + Insertions.insert( + clang::tooling::Replacement(Filename, FirstIncludeOffset, 0, Text)); + } + DEBUG(llvm::dbgs() << "Header insertions:\n"); + for (const auto &R : Insertions) { + DEBUG(llvm::dbgs() << R.toString() << "\n"); + } + + clang::format::FormatStyle Style = + clang::format::getStyle("file", Filename, FallbackStyle); + clang::tooling::Replacements Replaces = + formatReplacements(Code, Insertions, Style); + // FIXME: remove this when `clang::tooling::Replacements` is implemented as + // `std::vector`. + std::vector Results; + std::copy(Replaces.begin(), Replaces.end(), std::back_inserter(Results)); + return Results; + } + /// Generate replacements for the suggested includes. /// \return true if changes will be made, false otherwise. bool Rewrite(clang::SourceManager &SourceManager, clang::HeaderSearch &HeaderSearch, - std::vector &replacements) { + std::set &Headers, + std::vector &Replacements) { if (Untried.empty()) return false; const auto &ToTry = UntriedList.front(); - std::string ToAdd = "#include " + - minimizeInclude(ToTry, SourceManager, HeaderSearch) + - "\n"; - DEBUG(llvm::dbgs() << "Adding " << ToAdd << "\n"); + Headers.insert(minimizeInclude(ToTry, SourceManager, HeaderSearch)); - if (FirstIncludeOffset == -1U) - FirstIncludeOffset = 0; - - replacements.push_back(clang::tooling::Replacement( - SourceManager, FileBegin.getLocWithOffset(FirstIncludeOffset), 0, - ToAdd)); + StringRef Code = SourceManager.getBufferData(SourceManager.getMainFileID()); + Replacements = CreateReplacementsForHeaders(Code, Headers); // We currently abort after the first inserted include. The more // includes we have the less safe this becomes due to error recovery @@ -314,6 +351,10 @@ /// be used as the insertion point for new include directives. unsigned FirstIncludeOffset = -1U; + /// The fallback format style for formatting after insertion if there is no + /// clang-format config file found. + std::string FallbackStyle; + /// Includes we have left to try. A set to unique them and a list to keep /// track of the order. We prefer includes that were discovered early to avoid /// getting caught in results from error recovery. @@ -365,11 +406,12 @@ } // namespace IncludeFixerActionFactory::IncludeFixerActionFactory( - SymbolIndexManager &SymbolIndexMgr, - std::vector &Replacements, + SymbolIndexManager &SymbolIndexMgr, std::set &Headers, + std::vector &Replacements, StringRef StyleName, bool MinimizeIncludePaths) - : SymbolIndexMgr(SymbolIndexMgr), Replacements(Replacements), - MinimizeIncludePaths(MinimizeIncludePaths) {} + : SymbolIndexMgr(SymbolIndexMgr), Headers(Headers), + Replacements(Replacements), MinimizeIncludePaths(MinimizeIncludePaths), + FallbackStyle(StyleName) {} IncludeFixerActionFactory::~IncludeFixerActionFactory() = default; @@ -395,14 +437,14 @@ Compiler.getDiagnostics().setErrorLimit(0); // Run the parser, gather missing includes. - auto ScopedToolAction = - llvm::make_unique(SymbolIndexMgr, MinimizeIncludePaths); + auto ScopedToolAction = llvm::make_unique( + SymbolIndexMgr, FallbackStyle, MinimizeIncludePaths); Compiler.ExecuteAction(*ScopedToolAction); // Generate replacements. ScopedToolAction->Rewrite(Compiler.getSourceManager(), Compiler.getPreprocessor().getHeaderSearchInfo(), - Replacements); + Headers, Replacements); // Technically this should only return true if we're sure that we have a // parseable file. We don't know that though. Only inform users of fatal Index: clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp +++ clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp @@ -56,6 +56,12 @@ "used for editor integration."), cl::init(false), cl::cat(IncludeFixerCategory)); +cl::opt + Style("style", + cl::desc("Fallback style for reformatting after inserting new " + "headers if there is no clang-format config file found."), + cl::init("llvm"), cl::cat(IncludeFixerCategory)); + int includeFixerMain(int argc, const char **argv) { tooling::CommonOptionsParser options(argc, argv, IncludeFixerCategory); tooling::ClangTool tool(options.getCompilations(), @@ -133,9 +139,10 @@ } // Now run our tool. + std::set Headers; // Headers to be added. std::vector Replacements; include_fixer::IncludeFixerActionFactory Factory( - *SymbolIndexMgr, Replacements, MinimizeIncludePaths); + *SymbolIndexMgr, Headers, Replacements, Style, MinimizeIncludePaths); if (tool.run(&Factory) != 0) { llvm::errs() @@ -144,8 +151,8 @@ } if (!Quiet) - for (const tooling::Replacement &Replacement : Replacements) - llvm::errs() << "Added " << Replacement.getReplacementText(); + for (const auto &Header : Headers) + llvm::errs() << "Added #include " << Header; // Set up a new source manager for applying the resulting replacements. IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions); @@ -155,11 +162,10 @@ Diagnostics.setClient(&DiagnosticPrinter, false); if (STDINMode) { - for (const tooling::Replacement &Replacement : Replacements) { - FileID ID = SM.getMainFileID(); - unsigned LineNum = SM.getLineNumber(ID, Replacement.getOffset()); - llvm::outs() << LineNum << "," << Replacement.getReplacementText(); - } + tooling::Replacements Replaces(Replacements.begin(), Replacements.end()); + std::string ChangedCode = + tooling::applyAllReplacements(Code->getBuffer(), Replaces); + llvm::outs() << ChangedCode; return 0; } Index: clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py =================================================================== --- clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py +++ clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py @@ -16,6 +16,7 @@ # or save any files. To revert a fix, just undo. import argparse +import difflib import subprocess import sys import vim @@ -54,12 +55,10 @@ if stdout: lines = stdout.splitlines() - for line in lines: - line_num, text = line.split(",") - # clang-include-fixer provides 1-based line number - line_num = int(line_num) - 1 - print 'Inserting "{0}" at line {1}'.format(text, line_num) - vim.current.buffer[line_num:line_num] = [text] + sequence = difflib.SequenceMatcher(None, vim.current.buffer, lines) + for op in reversed(sequence.get_opcodes()): + if op[0] is not 'equal': + vim.current.buffer[op[1]:op[2]] = lines[op[3]:op[4]] if __name__ == '__main__': main() Index: clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp =================================================================== --- clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp +++ clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp @@ -70,8 +70,10 @@ SymbolIndexMgr->addSymbolIndex( llvm::make_unique(Symbols)); + std::set Headers; std::vector Replacements; - IncludeFixerActionFactory Factory(*SymbolIndexMgr, Replacements); + IncludeFixerActionFactory Factory(*SymbolIndexMgr, Headers, Replacements, + "llvm"); runOnCode(&Factory, Code, "input.cc", ExtraArgs); clang::RewriterTestContext Context; clang::FileID ID = Context.createInMemoryFile("input.cc", Code); @@ -80,32 +82,32 @@ } TEST(IncludeFixer, Typo) { - EXPECT_EQ("#include \nstd::string foo;\n", + EXPECT_EQ("#include \n\nstd::string foo;\n", runIncludeFixer("std::string foo;\n")); EXPECT_EQ( - "// comment\n#include \n#include \"foo.h\"\nstd::string foo;\n" + "// comment\n#include \"foo.h\"\n#include \nstd::string foo;\n" "#include \"dir/bar.h\"\n", runIncludeFixer("// comment\n#include \"foo.h\"\nstd::string foo;\n" "#include \"dir/bar.h\"\n")); - EXPECT_EQ("#include \n#include \"foo.h\"\nstd::string foo;\n", + EXPECT_EQ("#include \"foo.h\"\n#include \nstd::string foo;\n", runIncludeFixer("#include \"foo.h\"\nstd::string foo;\n")); EXPECT_EQ( - "#include \n#include \"foo.h\"\nstd::string::size_type foo;\n", + "#include \"foo.h\"\n#include \nstd::string::size_type foo;\n", runIncludeFixer("#include \"foo.h\"\nstd::string::size_type foo;\n")); // string without "std::" can also be fixed since fixed db results go through // SymbolIndexManager, and SymbolIndexManager matches unqualified identifiers // too. - EXPECT_EQ("#include \nstring foo;\n", + EXPECT_EQ("#include \n\nstring foo;\n", runIncludeFixer("string foo;\n")); } TEST(IncludeFixer, IncompleteType) { EXPECT_EQ( - "#include \n#include \"foo.h\"\n" + "#include \"foo.h\"\n#include \n" "namespace std {\nclass string;\n}\nstring foo;\n", runIncludeFixer("#include \"foo.h\"\n" "namespace std {\nclass string;\n}\nstring foo;\n")); @@ -113,26 +115,26 @@ TEST(IncludeFixer, MinimizeInclude) { std::vector IncludePath = {"-Idir/"}; - EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n", + EXPECT_EQ("#include \"otherdir/qux.h\"\n\na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); IncludePath = {"-isystemdir"}; - EXPECT_EQ("#include \na::b::foo bar;\n", + EXPECT_EQ("#include \n\na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); IncludePath = {"-iquotedir"}; - EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n", + EXPECT_EQ("#include \"otherdir/qux.h\"\n\na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); IncludePath = {"-Idir", "-Idir/otherdir"}; - EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n", + EXPECT_EQ("#include \"qux.h\"\n\na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); } TEST(IncludeFixer, NestedName) { // Some tests don't pass for target *-win32. std::vector args = {"-target", "x86_64-unknown-unknown"}; - EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n" + EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n\n" "int x = a::b::foo(0);\n", runIncludeFixer("int x = a::b::foo(0);\n", args)); @@ -142,36 +144,52 @@ EXPECT_EQ("#define FOO(x) a::##x\nint x = FOO(b::foo);\n", runIncludeFixer("#define FOO(x) a::##x\nint x = FOO(b::foo);\n")); - EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n" + EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n\n" "namespace a {}\nint a = a::b::foo(0);\n", runIncludeFixer("namespace a {}\nint a = a::b::foo(0);\n", args)); } TEST(IncludeFixer, MultipleMissingSymbols) { - EXPECT_EQ("#include \nstd::string bar;\nstd::sting foo;\n", + EXPECT_EQ("#include \n\nstd::string bar;\nstd::sting foo;\n", runIncludeFixer("std::string bar;\nstd::sting foo;\n")); } TEST(IncludeFixer, ScopedNamespaceSymbols) { - EXPECT_EQ("#include \"bar.h\"\nnamespace a { b::bar b; }\n", - runIncludeFixer("namespace a { b::bar b; }\n")); - EXPECT_EQ("#include \"bar.h\"\nnamespace A { a::b::bar b; }\n", - runIncludeFixer("namespace A { a::b::bar b; }\n")); - EXPECT_EQ("#include \"bar.h\"\nnamespace a { void func() { b::bar b; } }\n", - runIncludeFixer("namespace a { void func() { b::bar b; } }\n")); + EXPECT_EQ("#include \"bar.h\"\n\nnamespace a {\nb::bar b;\n}", + runIncludeFixer("namespace a {\nb::bar b;\n}")); + EXPECT_EQ("#include \"bar.h\"\n\nnamespace A {\na::b::bar b;\n}", + runIncludeFixer("namespace A {\na::b::bar b;\n}")); + EXPECT_EQ("#include \"bar.h\"\n\nnamespace a {\nvoid func() { b::bar b; }\n}", + runIncludeFixer("namespace a {\nvoid func() { b::bar b; }\n}")); EXPECT_EQ("namespace A { c::b::bar b; }\n", runIncludeFixer("namespace A { c::b::bar b; }\n")); // FIXME: The header should not be added here. Remove this after we support // full match. - EXPECT_EQ("#include \"bar.h\"\nnamespace A { b::bar b; }\n", - runIncludeFixer("namespace A { b::bar b; }\n")); + EXPECT_EQ("#include \"bar.h\"\n\nnamespace A {\nb::bar b;\n}", + runIncludeFixer("namespace A {\nb::bar b;\n}")); } TEST(IncludeFixer, EnumConstantSymbols) { - EXPECT_EQ("#include \"color.h\"\nint test = a::b::Green;\n", + EXPECT_EQ("#include \"color.h\"\n\nint test = a::b::Green;\n", runIncludeFixer("int test = a::b::Green;\n")); } +// FIXME: add test cases for inserting and sorting multiple headers when +// include-fixer supports multiple headers insertion. +TEST(IncludeFixer, InsertAndSortSingleHeader) { + // Insert one header. + std::string Code = "#include \"a.h\"\n" + "#include \"foo.h\"\n" + "\n" + "namespace a { b::bar b; }"; + std::string Expected = "#include \"a.h\"\n" + "#include \"bar.h\"\n" + "#include \"foo.h\"\n" + "\n" + "namespace a { b::bar b; }"; + EXPECT_EQ(Expected, runIncludeFixer(Code)); +} + } // namespace } // namespace include_fixer } // namespace clang