This revision was automatically updated to reflect the committed changes.
Closed by commit rL271287: [include-fixer] use clang-format cleaner to insert 
header. (authored by ioeric).

Changed prior to commit:
  http://reviews.llvm.org/D20816?vs=59067&id=59069#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20816

Files:
  clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp
  clang-tools-extra/trunk/include-fixer/IncludeFixer.h
  clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h
  clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp
  clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp

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,21 +78,22 @@
     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);
   return Context.getRewrittenText(ID);
 }
 
 TEST(IncludeFixer, Typo) {
-  EXPECT_EQ("#include <string>\n\nstd::string foo;\n",
+  EXPECT_EQ("#include <string>\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 <string>\nstd::string foo;\n"
-      "#include \"dir/bar.h\"\n",
+      "// comment\n#include \"foo.h\"\nstd::string foo;\n"
+      "#include \"dir/bar.h\"\n#include <string>\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 <string>\n\nstring foo;\n",
+  EXPECT_EQ("#include <string>\nstring foo;\n",
             runIncludeFixer("string foo;\n"));
 
   // Fully qualified name.
-  EXPECT_EQ("#include <string>\n\n::std::string foo;\n",
+  EXPECT_EQ("#include <string>\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<std::string> 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 <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 = {"-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 <string>\n\nstd::string bar;\nstd::sting foo;\n",
+  EXPECT_EQ("#include <string>\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"));
 }
 
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<PreprocessorHooks>(this));
     return llvm::make_unique<clang::ASTConsumer>();
   }
 
@@ -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,17 +199,13 @@
       return FixerContext;
 
     FixerContext.SymbolIdentifer = QuerySymbol;
-    FixerContext.FirstIncludeOffset = FirstIncludeOffset;
     for (const auto &Header : SymbolQueryResults)
       FixerContext.Headers.push_back(
           minimizeInclude(Header, SourceManager, HeaderSearch));
 
     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/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/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<std::string> Headers;
-  /// \brief The insertion point for new include header.
-  unsigned FirstIncludeOffset;
 };
 
 } // namespace include_fixer
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to