[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.

594

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

608

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;
...

613

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.

137

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.

613

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

[ioeric]

clang-move/ClangMove.cpp

606

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

607

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

609

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

616

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

651

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, additional #include "new.h" will
+    // be added in old.h.
+    bool OldDependOnNew = false;
+    // Whether new.h depends on old.h. If true, additional #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; }
+  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.
+  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) {
+  if (Spec.OldDependOnNew && Spec.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;
+  }
   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) {
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() {
+  const SourceManager* SM = nullptr;
   for (const auto &MovedDecl : RemovedDecls) {
-    const auto &SM = *MovedDecl.SM;
-    auto Range = GetFullRange(&SM, MovedDecl.Decl);
+    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";
+  }
+
+  if (SM) {
+    for (auto& It: FileToReplacements) {
+      StringRef FilePath = It.first;
+      if (Spec.OldDependOnNew &&
+          MakeAbsolutePath(*SM, FilePath) == makeAbsolutePath(Spec.OldHeader)) {
+        std::string IncludeNewH = "#include \""  + Spec.NewHeader + "\"\n";
+        auto Err = It.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);
+      if (SI == FilePathToFileID.end())
+        continue;
+      FileID ID = SI->second;
+      llvm::StringRef Code = SM->getBufferData(ID);
       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 = "#include \"" + Spec.OldHeader + "\"\n";
     FileToReplacements[Spec.NewHeader] = createInsertedReplacements(
-        HeaderIncludes, NewHeaderDecls, Spec.NewHeader, /*IsHeader=*/true);
+        HeaderIncludes, NewHeaderDecls, Spec.NewHeader, /*IsHeader=*/true,
+        Spec.NewDependOnOld ? 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 depends on new header. If true, clan-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 depends on old header. If true, clan-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) {
   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 std::string TestHeader = "class A {\npublic:\n  int f();\n};\n"
+                                 "class B {};\n";
+  const std::string TestCode = "#include \"foo.h\"\n";
+  const std::string ExpectedTestHeader = "#include \"new_foo.h\"\nclass B {};\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.c_str(), TestCode.c_str());
+  EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
+}
+
+TEST(ClangMove, AddDependentOldHeader) {
+  const char TestHeader[] = "class A {\npublic:\n  int f();\n};\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";
+  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]);
+}
+
 } // namespace
 } // namespce move
 } // namespace clang