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