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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
