[clang-move] The new.cc file should include new_header.h instead of old_header.h

[hokein]

Summary

Previously, all #includes (includeing old_header.h) in old.cc will be copied to new.cc,
however, the new.cc should include new_header.h instead of the old_header.h

Before applying the patch, the new.cc looks like:

#include "old_header.h"
...

The new.cc looks like with this patch:

#include "new_header"
...

[ioeric]

Could you elaborate on what this CL does in the description?

[ioeric]

Lg with nits.

clang-move/ClangMove.cpp

290 ↗

(On Diff #72165)

I think we also need a FIXME here. There could be interesting dependency: if the new target depends on the old target, then it makes sense to add the header.

295 ↗

(On Diff #72165)

I think ("#include <" + IncludeHeader + ">\n").str() is more efficient since it only creates one string instance.

[hokein]

Add comments.

clang-tools-extra/trunk/clang-move/ClangMove.h

public:
   void registerMatchers(ast_matchers::MatchFinder *Finder);
 
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
   void onEndOfTranslationUnit() override;
 
   // Add #includes from old.h/cc files. The FileName is where the #include
   // comes from.
-  void addIncludes(llvm::StringRef IncludeLine, llvm::StringRef FileName);
+  void addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
+                   llvm::StringRef FileName);
 
 private:
   void removeClassDefinitionInOldFiles();
   void moveClassDefinitionToNewFiles();
 
   MoveDefinitionSpec Spec;
   // The Key is file path, value is the replacements being applied to the file.
   std::map<std::string, tooling::Replacements> &FileToReplacements;

clang-tools-extra/trunk/clang-move/ClangMove.cpp

public:
 
   void InclusionDirective(clang::SourceLocation HashLoc,
                           const clang::Token & /*IncludeTok*/,
                           StringRef FileName, bool IsAngled,
                           clang::CharSourceRange /*FilenameRange*/,
                           const clang::FileEntry * /*File*/,
                           StringRef /*SearchPath*/, StringRef /*RelativePath*/,
                           const clang::Module * /*Imported*/) override {
-    if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc))) {
-      if (IsAngled) {
-        MoveTool->addIncludes("#include <" + FileName.str() + ">\n",
-                              FileEntry->getName());
-      } else {
-        MoveTool->addIncludes("#include \"" + FileName.str() + "\"\n",
-                              FileEntry->getName());
-      }
-    }
+    if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc)))
+      MoveTool->addIncludes(FileName, IsAngled, FileEntry->getName());
   }
 
 private:
   const SourceManager &SM;
   ClangMoveTool *const MoveTool;
 };
 
 clang::tooling::Replacement

 clang::tooling::Replacements
 createInsertedReplacements(const std::vector<std::string> &Includes,
                            const std::vector<ClangMoveTool::MovedDecl> &Decls,
                            llvm::StringRef FileName) {
   clang::tooling::Replacements InsertedReplacements;
 
   // Add #Includes.
   std::string AllIncludesString;
-  // FIXME: Filter out the old_header.h and add header guard.
+  // FIXME: Add header guard.
   for (const auto &Include : Includes)
     AllIncludesString += Include;
   clang::tooling::Replacement InsertInclude(FileName, 0, 0, AllIncludesString);
   addOrMergeReplacement(InsertInclude, &InsertedReplacements);
 
   // Add moved class definition and its related declarations. All declarations
   // in same namespace are grouped together.
   std::vector<std::string> CurrentNamespaces;

 }
 
 
 ClangMoveTool::ClangMoveTool(
       const MoveDefinitionSpec &MoveSpec,
       std::map<std::string, tooling::Replacements> &FileToReplacements)
       : Spec(MoveSpec), FileToReplacements(FileToReplacements) {
   Spec.Name = llvm::StringRef(Spec.Name).ltrim(':');
+  if (!Spec.NewHeader.empty())
+    CCIncludes.push_back("#include \"" + Spec.NewHeader + "\"\n");
 }
 
 void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   std::string FullyQualifiedName = "::" + Spec.Name;
   auto InOldHeader = isExpansionInFileMatching(Spec.OldHeader);
   auto InOldCC = isExpansionInFileMatching(Spec.OldCC);
   auto InOldFiles = anyOf(InOldHeader, InOldCC);
   auto InMovedClass =
if (RemovedDecls.empty())
                  "decls_in_anonymous_ns")) {
     MovedDecls.emplace_back(FD, &Result.Context->getSourceManager());
   } else if (const auto *ND =
                  Result.Nodes.getNodeAs<clang::NamedDecl>("static_decls")) {
     MovedDecls.emplace_back(ND, &Result.Context->getSourceManager());
   }
 }
 
-void ClangMoveTool::addIncludes(llvm::StringRef IncludeLine,
+void ClangMoveTool::addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
                                 llvm::StringRef FileName) {
+  // FIXME: Add old.h to the new.cc/h when the new target has dependencies on
+  // old.h/c. For instance, when moved class uses another class defined in
+  // old.h, the old.h should be added in new.h.
+  if (!Spec.OldHeader.empty() &&
+      llvm::StringRef(Spec.OldHeader).endswith(IncludeHeader))
+    return;
+
+  std::string IncludeLine =
+      IsAngled ? ("#include <" + IncludeHeader + ">\n").str()
+               : ("#include \"" + IncludeHeader + "\"\n").str();
   if (!Spec.OldHeader.empty() && FileName.endswith(Spec.OldHeader))
-    HeaderIncludes.push_back(IncludeLine.str());
+    HeaderIncludes.push_back(IncludeLine);
   else if (!Spec.OldCC.empty() && FileName.endswith(Spec.OldCC))
-    CCIncludes.push_back(IncludeLine.str());
+    CCIncludes.push_back(IncludeLine);
 }
 
 void ClangMoveTool::removeClassDefinitionInOldFiles() {
   for (const auto &MovedDecl : RemovedDecls) {
     auto EndLoc = getLocForEndOfDecl(MovedDecl.Decl, MovedDecl.SM);
     clang::tooling::Replacement RemoveReplacement(
         *MovedDecl.SM, clang::CharSourceRange::getTokenRange(
                            MovedDecl.Decl->getLocStart(), EndLoc),

clang-tools-extra/trunk/unittests/clang-move/ClangMoveTests.cpp

const char ExpectedNewHeader[] = "namespace a {\n"
                                  "\n"
                                  "private:\n"
                                  "  C1 *c1;\n"
                                  "  static int b;\n"
                                  "};\n"
                                  "} // namespace b\n"
                                  "} // namespace a\n";
 
-const char ExpectedNewCC[] = "#include \"foo.h\"\n"
-                             "namespace a {\n"
+const char ExpectedNewCC[] = "namespace a {\n"
                              "namespace b {\n"
                              "namespace {\n"
                              "void f1() {}\n"
                              "int kConstInt1 = 0;\n"
                              "} // namespace\n"
                              "static int kConstInt2 = 1;\n"
                              "static int help() {\n"
                              "  int a = 0;\n"

 
 TEST(ClangMove, MoveHeaderAndCC) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = "a::b::Foo";
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
+  std::string ExpectedHeader = "#include \"" + Spec.NewHeader + "\"\n";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
   EXPECT_EQ(ExpectedTestCC, Results[Spec.OldCC]);
   EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
-  EXPECT_EQ(ExpectedNewCC, Results[Spec.NewCC]);
+  EXPECT_EQ(ExpectedHeader + ExpectedNewCC, Results[Spec.NewCC]);
 }
 
 TEST(ClangMove, MoveHeaderOnly) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = "a::b::Foo";
   Spec.OldHeader = "foo.h";
   Spec.NewHeader = "new_foo.h";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(2u, Results.size());
   EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
   EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
 }
 
 TEST(ClangMove, MoveCCOnly) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = "a::b::Foo";
   Spec.OldCC = "foo.cc";
   Spec.NewCC = "new_foo.cc";
+  std::string ExpectedHeader = "#include \"foo.h\"\n";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(2u, Results.size());
   EXPECT_EQ(ExpectedTestCC, Results[Spec.OldCC]);
-  EXPECT_EQ(ExpectedNewCC, Results[Spec.NewCC]);
+  EXPECT_EQ(ExpectedHeader + ExpectedNewCC, Results[Spec.NewCC]);
 }
 
 TEST(ClangMove, MoveNonExistClass) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Name = "NonExistFoo";
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(0u, Results.size());
 }
 
 } // namespace
 } // namespce move
 } // namespace clang