[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:604
+  // Post process of cleanup around all the replacements.
+  for (auto& It: FileToReplacements) {
+    StringRef FilePath = It.first;
----------------
Maybe call this `FileAndReplacements` instead of `It`, which is a bit confusing when reading the code below.


================
Comment at: clang-move/ClangMove.cpp:611
+      std::string IncludeNewH = "#include \""  + Spec.NewHeader + "\"\n";
+      auto Err = It.second.add(
+          tooling::Replacement(FilePath, UINT_MAX, 0, IncludeNewH));
----------------
Maybe add a comment here indicating that this replacement *will* be cleaned up at the end.


================
Comment at: clang-move/ClangMove.cpp:618
+    auto SI = FilePathToFileID.find(FilePath);
+    if (SI == FilePathToFileID.end()) continue;
+    FileID ID = SI->second;
----------------
Do we need to do something (e.g. error handling) instead of just `continue`? If `continue` is allowed, please add comment explaining why.


================
Comment at: clang-move/ClangMove.cpp:619
+    if (SI == FilePathToFileID.end()) continue;
+    FileID ID = SI->second;
+    llvm::StringRef Code = SM->getBufferData(ID);
----------------
Just use `SI->second` below since `ID` is only used once.


================
Comment at: clang-move/ClangMove.cpp:646
+  if (!Spec.NewHeader.empty()) {
+    std::string OldHeaderInclude = "#include \"" + Spec.OldHeader + "\"\n";
     FileToReplacements[Spec.NewHeader] = createInsertedReplacements(
----------------
Maybe just
```
std::string OldHeaderInclude = Spec.NewDependOnOld  ? "#include \"" + Spec.OldHeader + "\"\n" : ":";
```


================
Comment at: clang-move/tool/ClangMoveMain.cpp:67
+    cl::desc(
+        "Whether old header depends on new header. If true, clan-move will "
+        "add #include of new header to old header."),
----------------
s/depends/will depend/

This makes it clearer IMO. Same below.


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:435
+  auto Results = runClangMoveOnCode(Spec, TestHeader.c_str(), TestCode.c_str());
+  EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
+}
----------------
Also check that new does not depend on old?


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:458
+  auto Results = runClangMoveOnCode(Spec, TestHeader, TestCode);
+  EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
+}
----------------
Also check that old does not depend on new?


https://reviews.llvm.org/D26966