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

[Eric Liu via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-move/ClangMove.cpp:309
+                           llvm::StringRef FileName, bool IsHeader = false,
+                           StringRef OldHeaderInclude = "") {
   std::string NewCode;
----------------
How do you handle the case where a whole file is copied?


================
Comment at: clang-move/ClangMove.cpp:416
+void ClangMoveTool::addRemovedDecl(const MovedDecl &Decl) {
+  const auto &SM = *Decl.SM;
+  auto Loc = Decl.Decl->getLocation();
----------------
This function is not very straight-forward to me so need some comments on what and why.


================
Comment at: clang-move/ClangMove.cpp:424
 void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  if (Spec.OldDependOnNew && Spec.NewDependOnOld) {
+    llvm::errs() << "Provide either --old_depend_on_new or "
----------------
Please check this in main. Otherwise, the Finder will still be run without any matcher.


================
Comment at: clang-move/ClangMove.cpp:594
 void ClangMoveTool::removeClassDefinitionInOldFiles() {
+  const SourceManager* SM = nullptr;
   for (const auto &MovedDecl : RemovedDecls) {
----------------
This variable seems pretty ugly. Can you just use `MovedDecl.SM` in the loop instead. It is used two times anyway.


================
Comment at: clang-move/ClangMove.cpp:608
 
-    if (!CleanReplacements) {
-      llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
-      continue;
+  if (SM) {
+    for (auto& It: FileToReplacements) {
----------------
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;
...
```


================
Comment at: clang-move/ClangMove.cpp:613
+          MakeAbsolutePath(*SM, FilePath) == makeAbsolutePath(Spec.OldHeader)) {
+        std::string IncludeNewH = "#include \""  + Spec.NewHeader + "\"\n";
+        auto Err = It.second.add(
----------------
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"`


================
Comment at: clang-move/ClangMove.h:56
     std::string NewCC;
+    // Whether old.h depends on new.h. If true, additional #include "new.h" will
+    // be added in old.h.
----------------
`additional` seems redundant.


================
Comment at: clang-move/ClangMove.h:137
   clang::CharSourceRange OldHeaderIncludeRange;
+  /// Mapping from FilePath to FileID.
+  llvm::StringMap<FileID> FilePathToFileID;
----------------
You might want to explain what this is for and why it is needed.


https://reviews.llvm.org/D26966