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

(On Diff #78853)

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

416 ↗

(On Diff #78853)

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

424 ↗

(On Diff #78853)

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

594 ↗

(On Diff #78853)

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

608 ↗

(On Diff #78853)

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 ↗

(On Diff #78853)

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 ↗

(On Diff #78853)

additional seems redundant.

137 ↗

(On Diff #78853)

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

[hokein]

Address review comments.

clang-move/ClangMove.cpp

309 ↗

(On Diff #78853)

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

416 ↗

(On Diff #78853)

Added a description in the method declaration.

613 ↗

(On Diff #78853)

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

[ioeric]

clang-move/ClangMove.cpp

604 ↗

(On Diff #78874)

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

611 ↗

(On Diff #78874)

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

618 ↗

(On Diff #78874)

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

619 ↗

(On Diff #78874)

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

646 ↗

(On Diff #78874)

Maybe just

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

clang-move/tool/ClangMoveMain.cpp

67 ↗

(On Diff #78874)

s/depends/will depend/

This makes it clearer IMO. Same below.

unittests/clang-move/ClangMoveTests.cpp

435 ↗

(On Diff #78874)

Also check that new does not depend on old?

458 ↗

(On Diff #78874)

Also check that old does not depend on new?

[hokein]

Update comments.

[ioeric]

Lg.

unittests/clang-move/ClangMoveTests.cpp

424 ↗

(On Diff #79043)

Is this indented correctly? or just phabricator bug?

[hokein]

Fix code indentation.

clang-tools-extra/trunk/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-tools-extra/trunk/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;
   }
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-tools-extra/trunk/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);

clang-tools-extra/trunk/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