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 @@ -60,23 +60,17 @@ std::string FallbackStyle; }; -/// Create replacements for the header being inserted. The replacements will -/// insert a header before the first #include in \p Code, and sort all headers -/// with the given clang-format style. +/// Create replacements, which are generated by clang-format, for the header +/// insertion. /// /// \param Code The source code. /// \param FilePath The source file path. /// \param Header The header being inserted. -/// \param FirstIncludeOffset The insertion point for new include directives. -/// The default value -1U means inserting the header at the first line, and if -/// there is no #include block, it will create a header block by inserting a -/// newline. /// \param Style clang-format style being used. /// /// \return Replacements for inserting and sorting headers. tooling::Replacements createInsertHeaderReplacements( StringRef Code, StringRef FilePath, StringRef Header, - unsigned FirstIncludeOffset = -1U, const clang::format::FormatStyle &Style = clang::format::getLLVMStyle()); } // 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 @@ -28,33 +28,6 @@ class Action; -class PreprocessorHooks : public clang::PPCallbacks { -public: - explicit PreprocessorHooks(Action *EnclosingPass) - : EnclosingPass(EnclosingPass), TrackedFile(nullptr) {} - - void FileChanged(clang::SourceLocation loc, - clang::PPCallbacks::FileChangeReason reason, - clang::SrcMgr::CharacteristicKind file_type, - clang::FileID prev_fid) override; - - void InclusionDirective(clang::SourceLocation HashLocation, - const clang::Token &IncludeToken, - llvm::StringRef FileName, bool IsAngled, - clang::CharSourceRange FileNameRange, - const clang::FileEntry *IncludeFile, - llvm::StringRef SearchPath, - llvm::StringRef relative_path, - const clang::Module *imported) override; - -private: - /// The current Action. - Action *EnclosingPass; - - /// The current FileEntry. - const clang::FileEntry *TrackedFile; -}; - /// Manages the parse, gathers include suggestions. class Action : public clang::ASTFrontendAction, public clang::ExternalSemaSource { @@ -67,8 +40,6 @@ CreateASTConsumer(clang::CompilerInstance &Compiler, StringRef InFile) override { Filename = InFile; - Compiler.getPreprocessor().addPPCallbacks( - llvm::make_unique(this)); return llvm::make_unique(); } @@ -195,22 +166,6 @@ StringRef filename() const { return Filename; } - /// Called for each include file we discover is in the file. - /// \param SourceManager the active SourceManager - /// \param canonical_path the canonical path to the include file - /// \param uttered_path the path as it appeared in the program - /// \param IsAngled whether angle brackets were used - /// \param HashLocation the source location of the include's \# - /// \param EndLocation the source location following the include - void NextInclude(clang::SourceManager *SourceManager, - llvm::StringRef canonical_path, llvm::StringRef uttered_path, - bool IsAngled, clang::SourceLocation HashLocation, - clang::SourceLocation EndLocation) { - unsigned Offset = SourceManager->getFileOffset(HashLocation); - if (FirstIncludeOffset == -1U) - FirstIncludeOffset = Offset; - } - /// Get the minimal include for a given path. std::string minimizeInclude(StringRef Include, const clang::SourceManager &SourceManager, @@ -244,7 +199,6 @@ return FixerContext; FixerContext.SymbolIdentifer = QuerySymbol; - FixerContext.FirstIncludeOffset = FirstIncludeOffset; for (const auto &Header : SymbolQueryResults) FixerContext.Headers.push_back( minimizeInclude(Header, SourceManager, HeaderSearch)); @@ -252,9 +206,6 @@ return FixerContext; } - /// Sets the location at the very top of the file. - void setFileBegin(clang::SourceLocation Location) { FileBegin = Location; } - private: /// Query the database for a given identifier. bool query(StringRef Query, SourceLocation Loc) { @@ -280,13 +231,6 @@ /// The absolute path to the file being processed. std::string Filename; - /// The location of the beginning of the tracked file. - clang::SourceLocation FileBegin; - - /// The offset of the last include in the original source file. This will - /// be used as the insertion point for new include directives. - unsigned FirstIncludeOffset = -1U; - /// The symbol being queried. std::string QuerySymbol; @@ -298,44 +242,6 @@ bool MinimizeIncludePaths = true; }; -void PreprocessorHooks::FileChanged(clang::SourceLocation Loc, - clang::PPCallbacks::FileChangeReason Reason, - clang::SrcMgr::CharacteristicKind FileType, - clang::FileID PrevFID) { - // Remember where the main file starts. - if (Reason == clang::PPCallbacks::EnterFile) { - clang::SourceManager *SourceManager = - &EnclosingPass->getCompilerInstance().getSourceManager(); - clang::FileID loc_id = SourceManager->getFileID(Loc); - if (const clang::FileEntry *FileEntry = - SourceManager->getFileEntryForID(loc_id)) { - if (FileEntry->getName() == EnclosingPass->filename()) { - EnclosingPass->setFileBegin(Loc); - TrackedFile = FileEntry; - } - } - } -} - -void PreprocessorHooks::InclusionDirective( - clang::SourceLocation HashLocation, const clang::Token &IncludeToken, - llvm::StringRef FileName, bool IsAngled, - clang::CharSourceRange FileNameRange, const clang::FileEntry *IncludeFile, - llvm::StringRef SearchPath, llvm::StringRef relative_path, - const clang::Module *imported) { - // Remember include locations so we can insert our new include at the end of - // the include block. - clang::SourceManager *SourceManager = - &EnclosingPass->getCompilerInstance().getSourceManager(); - auto IDPosition = SourceManager->getDecomposedExpansionLoc(HashLocation); - const FileEntry *SourceFile = - SourceManager->getFileEntryForID(IDPosition.first); - if (!IncludeFile || TrackedFile != SourceFile) - return; - EnclosingPass->NextInclude(SourceManager, IncludeFile->getName(), FileName, - IsAngled, HashLocation, FileNameRange.getEnd()); -} - } // namespace IncludeFixerActionFactory::IncludeFixerActionFactory( @@ -384,32 +290,17 @@ tooling::Replacements createInsertHeaderReplacements(StringRef Code, StringRef FilePath, - StringRef Header, unsigned FirstIncludeOffset, + StringRef Header, const clang::format::FormatStyle &Style) { if (Header.empty()) return tooling::Replacements(); - // 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(FilePath, FirstIncludeOffset, 0, "\n")); - } - // Keep inserting new headers before the first header. - std::string Text = "#include " + Header.str() + "\n"; - Insertions.insert( - clang::tooling::Replacement(FilePath, FirstIncludeOffset, 0, Text)); - DEBUG({ - llvm::dbgs() << "Header insertions:\n"; - for (const auto &R : Insertions) - llvm::dbgs() << R.toString() << '\n'; - }); + std::string IncludeName = "#include " + Header.str() + "\n"; + // Create replacements for the new header. + clang::tooling::Replacements Insertions = { + tooling::Replacement(FilePath, UINT_MAX, 0, IncludeName)}; - return formatReplacements(Code, Insertions, Style); + return formatReplacements( + Code, cleanupAroundReplacements(Code, Insertions, Style), Style); } } // namespace include_fixer Index: clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h +++ clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h @@ -22,8 +22,6 @@ std::string SymbolIdentifer; /// \brief The headers which have SymbolIdentifier definitions. std::vector Headers; - /// \brief The insertion point for new include header. - unsigned FirstIncludeOffset; }; } // namespace include_fixer 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 @@ -168,11 +168,9 @@ return 1; } - // FIXME: Insert the header in right FirstIncludeOffset. tooling::Replacements Replacements = clang::include_fixer::createInsertHeaderReplacements( - Code->getBuffer(), FilePath, InsertHeader, - /*FirstIncludeOffset=*/0, InsertStyle); + Code->getBuffer(), FilePath, InsertHeader, InsertStyle); tooling::Replacements Replaces(Replacements.begin(), Replacements.end()); std::string ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), Replaces); @@ -218,7 +216,7 @@ tooling::Replacements Replacements = clang::include_fixer::createInsertHeaderReplacements( /*Code=*/Buffer.get()->getBuffer(), FilePath, Context.Headers.front(), - Context.FirstIncludeOffset, InsertStyle); + InsertStyle); if (!Quiet) llvm::errs() << "Added #include" << Context.Headers.front(); 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 @@ -78,8 +78,7 @@ return Code; tooling::Replacements Replacements = clang::include_fixer::createInsertHeaderReplacements( - Code, "input.cc", FixerContext.Headers.front(), - FixerContext.FirstIncludeOffset); + Code, "input.cc", FixerContext.Headers.front()); clang::RewriterTestContext Context; clang::FileID ID = Context.createInMemoryFile("input.cc", Code); clang::tooling::applyAllReplacements(Replacements, Context.Rewrite); @@ -87,12 +86,14 @@ } TEST(IncludeFixer, Typo) { - EXPECT_EQ("#include \n\nstd::string foo;\n", + EXPECT_EQ("#include \nstd::string foo;\n", runIncludeFixer("std::string foo;\n")); + // FIXME: the current version of include-fixer does not get this test case + // right - header should be inserted before definition. EXPECT_EQ( - "// comment\n#include \"foo.h\"\n#include \nstd::string foo;\n" - "#include \"dir/bar.h\"\n", + "// comment\n#include \"foo.h\"\nstd::string foo;\n" + "#include \"dir/bar.h\"\n#include \n", runIncludeFixer("// comment\n#include \"foo.h\"\nstd::string foo;\n" "#include \"dir/bar.h\"\n")); @@ -106,11 +107,11 @@ // string without "std::" can also be fixed since fixed db results go through // SymbolIndexManager, and SymbolIndexManager matches unqualified identifiers // too. - EXPECT_EQ("#include \n\nstring foo;\n", + EXPECT_EQ("#include \nstring foo;\n", runIncludeFixer("string foo;\n")); // Fully qualified name. - EXPECT_EQ("#include \n\n::std::string foo;\n", + EXPECT_EQ("#include \n::std::string foo;\n", runIncludeFixer("::std::string foo;\n")); // Should not match std::string. EXPECT_EQ("::string foo;\n", runIncludeFixer("::string foo;\n")); @@ -126,24 +127,24 @@ TEST(IncludeFixer, MinimizeInclude) { std::vector IncludePath = {"-Idir/"}; - EXPECT_EQ("#include \"otherdir/qux.h\"\n\na::b::foo bar;\n", + EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); IncludePath = {"-isystemdir"}; - EXPECT_EQ("#include \n\na::b::foo bar;\n", + EXPECT_EQ("#include \na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); IncludePath = {"-iquotedir"}; - EXPECT_EQ("#include \"otherdir/qux.h\"\n\na::b::foo bar;\n", + EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); IncludePath = {"-Idir", "-Idir/otherdir"}; - EXPECT_EQ("#include \"qux.h\"\n\na::b::foo bar;\n", + EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n", runIncludeFixer("a::b::foo bar;\n", IncludePath)); } TEST(IncludeFixer, NestedName) { - EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n\n" + EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n" "int x = a::b::foo(0);\n", runIncludeFixer("int x = a::b::foo(0);\n")); @@ -153,33 +154,35 @@ 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\n" - "namespace a {}\nint a = a::b::foo(0);\n", + // The empty namespace is cleaned up by clang-format after include-fixer + // finishes. + EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n" + "\nint a = a::b::foo(0);\n", runIncludeFixer("namespace a {}\nint a = a::b::foo(0);\n")); } TEST(IncludeFixer, MultipleMissingSymbols) { - EXPECT_EQ("#include \n\nstd::string bar;\nstd::sting foo;\n", + EXPECT_EQ("#include \nstd::string bar;\nstd::sting foo;\n", runIncludeFixer("std::string bar;\nstd::sting foo;\n")); } TEST(IncludeFixer, ScopedNamespaceSymbols) { - EXPECT_EQ("#include \"bar.h\"\n\nnamespace a {\nb::bar b;\n}", + EXPECT_EQ("#include \"bar.h\"\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}", + EXPECT_EQ("#include \"bar.h\"\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}", + EXPECT_EQ("#include \"bar.h\"\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\"\n\nnamespace A {\nb::bar b;\n}", + EXPECT_EQ("#include \"bar.h\"\nnamespace A {\nb::bar b;\n}", runIncludeFixer("namespace A {\nb::bar b;\n}")); } TEST(IncludeFixer, EnumConstantSymbols) { - EXPECT_EQ("#include \"color.h\"\n\nint test = a::b::Green;\n", + EXPECT_EQ("#include \"color.h\"\nint test = a::b::Green;\n", runIncludeFixer("int test = a::b::Green;\n")); }