[clang-move] Add some options allowing to add old/new.h to new/old.h respectively.

[hokein]

Summary

• --new_depend_on_old: new header will include old header
• --old_depend_on_new: old header will include new header.

[ioeric]

clang-move/ClangMove.cpp

309

How do you handle the case where a whole file is copied?

416

This function is not very straight-forward to me so need some comments on what and why.

424

Please check this in main. Otherwise, the Finder will still be run without any matcher.

588

This variable seems pretty ugly. Can you just use MovedDecl.SM in the loop instead. It is used two times anyway.

629

What does this check mean? Please comment. It seems weird to rely on a variable that is set in a loop as a condition.

Also, can you just do this to save some indentation?

if (!SM) return;
...

634

We might want to do what include-fixer does to compute the best path to put in #include, e.g. we'd want #include "llvm/XXX.h" instead of #include "llvm/include/llvm/XXX.h"

clang-move/ClangMove.h

56

additional seems redundant.

140

You might want to explain what this is for and why it is needed.

[hokein]

Address review comments.

clang-move/ClangMove.cpp

309

Cases where a whole file is moved are handled in other functions, not in this function.

416

Added a description in the method declaration.

634

It might make sense, but it will require non-trivial change of this patch. Add a FIXME.

[ioeric]

clang-move/ClangMove.cpp

604

Maybe call this FileAndReplacements instead of It, which is a bit confusing when reading the code below.

611

Maybe add a comment here indicating that this replacement *will* be cleaned up at the end.

619

Do we need to do something (e.g. error handling) instead of just continue? If continue is allowed, please add comment explaining why.

620

Just use SI->second below since ID is only used once.

647

Maybe just

std::string OldHeaderInclude = Spec.NewDependOnOld  ? "#include \"" + Spec.OldHeader + "\"\n" : ":";

clang-move/tool/ClangMoveMain.cpp

67

s/depends/will depend/

This makes it clearer IMO. Same below.

unittests/clang-move/ClangMoveTests.cpp

435

Also check that new does not depend on old?

458

Also check that old does not depend on new?

[hokein]

Update comments.

[ioeric]

Lg.

unittests/clang-move/ClangMoveTests.cpp

424

Is this indented correctly? or just phabricator bug?

[hokein]

Fix code indentation.

clang-move/ClangMove.h

 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_MOVE_CLANGMOVE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_MOVE_CLANGMOVE_H
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/StringMap.h"
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace move {
 
struct MoveDefinitionSpec {
     // The file path of old header, can be relative path and absolute path.
     std::string OldHeader;
     // The file path of old cc, can be relative path and absolute path.
     std::string OldCC;
     // The file path of new header, can be relative path and absolute path.
     std::string NewHeader;
     // The file path of new cc, can be relative path and absolute path.
     std::string NewCC;
+    // Whether old.h depends on new.h. If true, #include "new.h" will be added
+    // in old.h.
+    bool OldDependOnNew = false;
+    // Whether new.h depends on old.h. If true, #include "old.h" will be added
+    // in new.h.
+    bool NewDependOnOld = false;
   };
 
   ClangMoveTool(
       const MoveDefinitionSpec &MoveSpec,
       std::map<std::string, tooling::Replacements> &FileToReplacements,
       llvm::StringRef OriginalRunningDirectory, llvm::StringRef Style);
 
   void registerMatchers(ast_matchers::MatchFinder *Finder);
public:
   /// \param SM The SourceManager.
   void addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
                    llvm::StringRef SearchPath, llvm::StringRef FileName,
                    clang::CharSourceRange IncludeFilenameRange,
                    const SourceManager &SM);
 
   std::vector<MovedDecl> &getMovedDecls() { return MovedDecls; }
 
-  std::vector<MovedDecl> &getRemovedDecls() { return RemovedDecls; }
+  /// Add declarations being removed from old.h/cc. For each declarations, the
+  /// method also records the mapping relationship between the corresponding
+  /// FilePath and its FileID.
+  void addRemovedDecl(const MovedDecl &Decl);
 
   llvm::SmallPtrSet<const NamedDecl *, 8> &getUnremovedDeclsInOldHeader() {
     return UnremovedDeclsInOldHeader;
   }
 
 private:
   // Make the Path absolute using the OrignalRunningDirectory if the Path is not
   // an absolute path. An empty Path will result in an empty string.
private:
   // The name of a predefined code style.
   std::string FallbackStyle;
   // The unmoved named declarations in old header.
   llvm::SmallPtrSet<const NamedDecl*, 8> UnremovedDeclsInOldHeader;
   /// The source range for the written file name in #include (i.e. "old.h" for
   /// #include "old.h") in old.cc,  including the enclosing quotes or angle
   /// brackets.
   clang::CharSourceRange OldHeaderIncludeRange;
+  /// Mapping from FilePath to FileID, which can be used in post processes like
+  /// cleanup around replacements.
+  llvm::StringMap<FileID> FilePathToFileID;
 };
 
 class ClangMoveAction : public clang::ASTFrontendAction {
 public:
   ClangMoveAction(
       const ClangMoveTool::MoveDefinitionSpec &spec,
       std::map<std::string, tooling::Replacements> &FileToReplacements,
       llvm::StringRef OriginalRunningDirectory, llvm::StringRef FallbackStyle)

clang-move/ClangMove.cpp

void run(const MatchFinder::MatchResult &Result) override {
     const auto *FD = Result.Nodes.getNodeAs<clang::FunctionDecl>("function");
     assert(FD);
     const clang::NamedDecl *D = FD;
     if (const auto *FTD = FD->getDescribedFunctionTemplate())
       D = FTD;
     MoveTool->getMovedDecls().emplace_back(D,
                                            &Result.Context->getSourceManager());
     MoveTool->getUnremovedDeclsInOldHeader().erase(D);
-    MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+    MoveTool->addRemovedDecl(MoveTool->getMovedDecls().back());
   }
 
 private:
   ClangMoveTool *MoveTool;
 };
 
 class ClassDeclarationMatch : public MatchFinder::MatchCallback {
 public:

 
 private:
   void MatchClassMethod(const clang::CXXMethodDecl* CMD,
                         clang::SourceManager* SM) {
     // Skip inline class methods. isInline() ast matcher doesn't ignore this
     // case.
     if (!CMD->isInlined()) {
       MoveTool->getMovedDecls().emplace_back(CMD, SM);
-      MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+      MoveTool->addRemovedDecl(MoveTool->getMovedDecls().back());
       // Get template class method from its method declaration as
       // UnremovedDecls stores template class method.
       if (const auto *FTD = CMD->getDescribedFunctionTemplate())
         MoveTool->getUnremovedDeclsInOldHeader().erase(FTD);
       else
         MoveTool->getUnremovedDeclsInOldHeader().erase(CMD);
     }
   }
 
   void MatchClassStaticVariable(const clang::NamedDecl *VD,
                                 clang::SourceManager* SM) {
     MoveTool->getMovedDecls().emplace_back(VD, SM);
-    MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+    MoveTool->addRemovedDecl(MoveTool->getMovedDecls().back());
     MoveTool->getUnremovedDeclsInOldHeader().erase(VD);
   }
 
   void MatchClassDeclaration(const clang::CXXRecordDecl *CD,
                              clang::SourceManager* SM) {
     // Get class template from its class declaration as UnremovedDecls stores
     // class template.
     if (const auto *TC = CD->getDescribedClassTemplate())
       MoveTool->getMovedDecls().emplace_back(TC, SM);
     else
       MoveTool->getMovedDecls().emplace_back(CD, SM);
-    MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+    MoveTool->addRemovedDecl(MoveTool->getMovedDecls().back());
     MoveTool->getUnremovedDeclsInOldHeader().erase(
         MoveTool->getMovedDecls().back().Decl);
   }
 
   ClangMoveTool *MoveTool;
 };
 
 // Expand to get the end location of the line where the EndLoc of the given
std::vector<std::string> GetNamespaces(const clang::Decl *D) {
   }
   std::reverse(Namespaces.begin(), Namespaces.end());
   return Namespaces;
 }
 
 clang::tooling::Replacements
 createInsertedReplacements(const std::vector<std::string> &Includes,
                            const std::vector<ClangMoveTool::MovedDecl> &Decls,
-                           llvm::StringRef FileName, bool IsHeader = false) {
+                           llvm::StringRef FileName, bool IsHeader = false,
+                           StringRef OldHeaderInclude = "") {
   std::string NewCode;
   std::string GuardName(FileName);
   if (IsHeader) {
     for (size_t i = 0; i < GuardName.size(); ++i) {
       if (!isAlphanumeric(GuardName[i]))
         GuardName[i] = '_';
     }
     GuardName = StringRef(GuardName).upper();
     NewCode += "#ifndef " + GuardName + "\n";
     NewCode += "#define " + GuardName + "\n\n";
   }
 
+  NewCode += OldHeaderInclude;
   // Add #Includes.
   for (const auto &Include : Includes)
     NewCode += Include;
 
   if (!Includes.empty())
     NewCode += "\n";
 
   // Add moved class definition and its related declarations. All declarations
ClangMoveTool::ClangMoveTool(
     llvm::StringRef OriginalRunningDirectory, llvm::StringRef FallbackStyle)
     : Spec(MoveSpec), FileToReplacements(FileToReplacements),
       OriginalRunningDirectory(OriginalRunningDirectory),
       FallbackStyle(FallbackStyle) {
   if (!Spec.NewHeader.empty())
     CCIncludes.push_back("#include \"" + Spec.NewHeader + "\"\n");
 }
 
+void ClangMoveTool::addRemovedDecl(const MovedDecl &Decl) {
+  const auto &SM = *Decl.SM;
+  auto Loc = Decl.Decl->getLocation();
+  StringRef FilePath = SM.getFilename(Loc);
+  FilePathToFileID[FilePath] = SM.getFileID(Loc);
+  RemovedDecls.push_back(Decl);
+}
+
 void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   Optional<ast_matchers::internal::Matcher<NamedDecl>> HasAnySymbolNames;
   for (StringRef SymbolName: Spec.Names) {
     llvm::StringRef GlobalSymbolName = SymbolName.trim().ltrim(':');
     const auto HasName = hasName(("::" + GlobalSymbolName).str());
     HasAnySymbolNames =
         HasAnySymbolNames ? anyOf(*HasAnySymbolNames, HasName) : HasName;
   }
   if (!HasAnySymbolNames) {
     llvm::errs() << "No symbols being moved.\n";
void ClangMoveTool::addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
   if (AbsoluteOldHeader == AbsoluteCurrentFile) {
     HeaderIncludes.push_back(IncludeLine);
   } else if (makeAbsolutePath(Spec.OldCC) == AbsoluteCurrentFile) {
     CCIncludes.push_back(IncludeLine);
   }
 }
 
 void ClangMoveTool::removeClassDefinitionInOldFiles() {
+  if (RemovedDecls.empty()) return;
   for (const auto &MovedDecl : RemovedDecls) {
     const auto &SM = *MovedDecl.SM;
     auto Range = GetFullRange(&SM, MovedDecl.Decl);
     clang::tooling::Replacement RemoveReplacement(
-        *MovedDecl.SM,
+        SM,
         clang::CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()),
         "");
     std::string FilePath = RemoveReplacement.getFilePath().str();
     auto Err = FileToReplacements[FilePath].add(RemoveReplacement);
-    if (Err) {
+    if (Err)
+      llvm::errs() << llvm::toString(std::move(Err)) << "\n";
+  }
+  const SourceManager* SM = RemovedDecls[0].SM;
+
+  // Post process of cleanup around all the replacements.
+  for (auto& FileAndReplacements: FileToReplacements) {
+    StringRef FilePath = FileAndReplacements.first;
+    // Add #include of new header to old header.
+    if (Spec.OldDependOnNew &&
+        MakeAbsolutePath(*SM, FilePath) == makeAbsolutePath(Spec.OldHeader)) {
+      // FIXME: Minimize the include path like include-fixer.
+      std::string IncludeNewH = "#include \""  + Spec.NewHeader + "\"\n";
+      // This replacment for inserting header will be cleaned up at the end.
+      auto Err = FileAndReplacements.second.add(
+          tooling::Replacement(FilePath, UINT_MAX, 0, IncludeNewH));
+      if (Err)
         llvm::errs() << llvm::toString(std::move(Err)) << "\n";
-      continue;
     }
 
-    llvm::StringRef Code =
-        SM.getBufferData(SM.getFileID(MovedDecl.Decl->getLocation()));
+    auto SI = FilePathToFileID.find(FilePath);
+    // Ignore replacements for new.h/cc.
+    if (SI == FilePathToFileID.end()) continue;
+    llvm::StringRef Code = SM->getBufferData(SI->second);
     format::FormatStyle Style =
         format::getStyle("file", FilePath, FallbackStyle);
     auto CleanReplacements = format::cleanupAroundReplacements(
         Code, FileToReplacements[FilePath], Style);
 
     if (!CleanReplacements) {
       llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
       continue;
     }
     FileToReplacements[FilePath] = *CleanReplacements;
   }
 }
 
 void ClangMoveTool::moveClassDefinitionToNewFiles() {
   std::vector<MovedDecl> NewHeaderDecls;
   std::vector<MovedDecl> NewCCDecls;
   for (const auto &MovedDecl : MovedDecls) {
     if (isInHeaderFile(*MovedDecl.SM, MovedDecl.Decl, OriginalRunningDirectory,
                        Spec.OldHeader))
       NewHeaderDecls.push_back(MovedDecl);
     else
       NewCCDecls.push_back(MovedDecl);
   }
 
-  if (!Spec.NewHeader.empty())
+  if (!Spec.NewHeader.empty()) {
+    std::string OldHeaderInclude =
+        Spec.NewDependOnOld ? "#include \"" + Spec.OldHeader + "\"\n" : "";
     FileToReplacements[Spec.NewHeader] = createInsertedReplacements(
-        HeaderIncludes, NewHeaderDecls, Spec.NewHeader, /*IsHeader=*/true);
+        HeaderIncludes, NewHeaderDecls, Spec.NewHeader, /*IsHeader=*/true,
+        OldHeaderInclude);
+  }
   if (!Spec.NewCC.empty())
     FileToReplacements[Spec.NewCC] =
         createInsertedReplacements(CCIncludes, NewCCDecls, Spec.NewCC);
 }
 
 // Move all contents from OldFile to NewFile.
 void ClangMoveTool::moveAll(SourceManager &SM, StringRef OldFile,
                             StringRef NewFile) {

clang-move/tool/ClangMoveMain.cpp

cl::opt<std::string>
     NewHeader("new_header",
               cl::desc("The relative/absolute file path of new header."),
               cl::cat(ClangMoveCategory));
 
 cl::opt<std::string>
     NewCC("new_cc", cl::desc("The relative/absolute file path of new cc."),
           cl::cat(ClangMoveCategory));
 
+cl::opt<bool> OldDependOnNew(
+    "old_depend_on_new",
+    cl::desc(
+        "Whether old header will depend on new header. If true, clang-move will "
+        "add #include of new header to old header."),
+    cl::init(false), cl::cat(ClangMoveCategory));
+
+cl::opt<bool> NewDependOnOld(
+    "new_depend_on_old",
+    cl::desc(
+        "Whether new header will depend on old header. If true, clang-move will "
+        "add #include of old header to new header."),
+    cl::init(false), cl::cat(ClangMoveCategory));
+
 cl::opt<std::string>
     Style("style",
           cl::desc("The style name used for reformatting. Default is \"llvm\""),
           cl::init("llvm"), cl::cat(ClangMoveCategory));
 
 cl::opt<bool> Dump("dump_result",
                    cl::desc("Dump results in JSON format to stdout."),
                    cl::cat(ClangMoveCategory));
int main(int argc, const char **argv) {
   std::unique_ptr<const char *[]> RawExtraArgs(
       new const char *[ExtraArgs.size()]);
   for (size_t i = 0; i < ExtraArgs.size(); ++i)
     RawExtraArgs[i] = ExtraArgs[i].c_str();
   int Argc = argc + 1;
   tooling::CommonOptionsParser OptionsParser(Argc, RawExtraArgs.get(),
                                              ClangMoveCategory);
 
+  if (OldDependOnNew && NewDependOnOld) {
+    llvm::errs() << "Provide either --old_depend_on_new or "
+                    "--new_depend_on_old. clang-move doesn't support these two "
+                    "options at same time (It will introduce include cycle).\n";
+    return 1;
+  }
+
   tooling::RefactoringTool Tool(OptionsParser.getCompilations(),
                                 OptionsParser.getSourcePathList());
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Names = {Names.begin(), Names.end()};
   Spec.OldHeader = OldHeader;
   Spec.NewHeader = NewHeader;
   Spec.OldCC = OldCC;
   Spec.NewCC = NewCC;
+  Spec.OldDependOnNew = OldDependOnNew;
+  Spec.NewDependOnOld = NewDependOnOld;
 
   llvm::SmallString<128> InitialDirectory;
   if (std::error_code EC = llvm::sys::fs::current_path(InitialDirectory))
     llvm::report_fatal_error("Cannot detect current path: " +
                              Twine(EC.message()));
 
   auto Factory = llvm::make_unique<clang::move::ClangMoveActionFactory>(
       Spec, Tool.getReplacements(), InitialDirectory.str(), Style);

unittests/clang-move/ClangMoveTests.cpp

TEST(ClangMove, WellFormattedCode) {
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   auto Results = runClangMoveOnCode(Spec, TestHeader.c_str(), TestCode.c_str());
   EXPECT_EQ(ExpectedNewCC, Results[Spec.NewCC]);
   EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
 }
 
+TEST(ClangMove, AddDependentNewHeader) {
+  const char TestHeader[] = "class A {};\n"
+                            "class B {};\n";
+  const char TestCode[] = "#include \"foo.h\"\n";
+  const char ExpectedOldHeader[] = "#include \"new_foo.h\"\nclass B {};\n";
+  const char ExpectedNewHeader[] = "#ifndef NEW_FOO_H\n"
+                                   "#define NEW_FOO_H\n"
+                                   "\n"
+                                   "class A {};\n"
+                                   "\n"
+                                   "#endif // NEW_FOO_H\n";
+  move::ClangMoveTool::MoveDefinitionSpec Spec;
+  Spec.Names.push_back("A");
+  Spec.OldHeader = "foo.h";
+  Spec.OldCC = "foo.cc";
+  Spec.NewHeader = "new_foo.h";
+  Spec.NewCC = "new_foo.cc";
+  Spec.OldDependOnNew = true;
+  auto Results = runClangMoveOnCode(Spec, TestHeader, TestCode);
+  EXPECT_EQ(ExpectedOldHeader, Results[Spec.OldHeader]);
+  EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
+}
+
+TEST(ClangMove, AddDependentOldHeader) {
+  const char TestHeader[] = "class A {};\n"
+                            "class B {};\n";
+  const char TestCode[] = "#include \"foo.h\"\n";
+  const char ExpectedNewHeader[] = "#ifndef NEW_FOO_H\n"
+                                   "#define NEW_FOO_H\n"
+                                   "\n"
+                                   "#include \"foo.h\"\n"
+                                   "\n"
+                                   "class B {};\n"
+                                   "\n"
+                                   "#endif // NEW_FOO_H\n";
+  const char ExpectedOldHeader[] = "class A {};\n";
+  move::ClangMoveTool::MoveDefinitionSpec Spec;
+  Spec.Names.push_back("B");
+  Spec.OldHeader = "foo.h";
+  Spec.OldCC = "foo.cc";
+  Spec.NewHeader = "new_foo.h";
+  Spec.NewCC = "new_foo.cc";
+  Spec.NewDependOnOld = true;
+  auto Results = runClangMoveOnCode(Spec, TestHeader, TestCode);
+  EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
+  EXPECT_EQ(ExpectedOldHeader, Results[Spec.OldHeader]);
+}
+
 } // namespace
 } // namespce move
 } // namespace clang