[PATCH] D24922: [clang-move] Make it support both relative and absolute file path arguments.

[Eric Liu via cfe-commits]

ioeric added inline comments.

================
Comment at: clang-move/ClangMove.cpp:33
@@ +32,3 @@
+  auto EC = llvm::sys::fs::make_absolute(InitialDirectory, AbsolutePath);
+  assert(!EC);
+  (void)EC;
----------------
This doesn't seems to be good error handling... maybe also print an error message in case this fails in release binary.

================
Comment at: clang-move/ClangMove.cpp:39
@@ +38,3 @@
+
+std::string MakeAbsolutePath(const SourceManager& SM, StringRef Path) {
+  llvm::SmallString<128> AbsolutePath(Path);
----------------
There are two versions of this. Please add comments about the differences.

================
Comment at: clang-move/ClangMove.cpp:43
@@ +42,3 @@
+      SM.getFileManager().getVirtualFileSystem()->makeAbsolute(AbsolutePath);
+  assert(!EC);
+  (void)EC;
----------------
Same as above. Error handling.

================
Comment at: clang-move/ClangMove.cpp:62
@@ +61,3 @@
+  return MakeAbsolutePath(SourceManager, FileEntry->getName()) ==
+         AbsoluteFilePath;
+}
----------------
To generalize the function, maybe also remove dots in `AbsoluteFilePath`?

================
Comment at: clang-move/ClangMove.cpp:78
@@ -38,2 +77,3 @@
     if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc)))
-      MoveTool->addIncludes(FileName, IsAngled, FileEntry->getName());
+      MoveTool->addIncludes(FileName, MakeAbsolutePath(SM, SearchPath),
+                            IsAngled, FileEntry->getName(), SM);
----------------
Looks like `MakeAbsolutePath(SM, SearchPath)` can be done in `addIncludes` since you are also passing `SM`.

================
Comment at: clang-move/ClangMove.cpp:109
@@ -68,3 +108,3 @@
 bool IsInHeaderFile(const clang::SourceManager &SM, const clang::Decl *D,
-                    llvm::StringRef HeaderFile) {
-  if (HeaderFile.empty())
+                    llvm::StringRef OldHeader,
+                    llvm::StringRef OriginalRunningDirectory) {
----------------
I think it would be easier to read if you put `OriginalRunningDirectory` before `OldHeader` .

================
Comment at: clang-move/ClangMove.h:63
@@ -57,3 +62,3 @@
   // comes from.
-  void addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
-                   llvm::StringRef FileName);
+  void addIncludes(llvm::StringRef IncludeHeader,
+                   llvm::StringRef SearchPath,
----------------
This function could use more comments. What are those parameters and what are they for?

================
Comment at: clang-move/ClangMove.h:64
@@ +63,3 @@
+  void addIncludes(llvm::StringRef IncludeHeader,
+                   llvm::StringRef SearchPath,
+                   bool IsAngled,
----------------
Should `SearchPath` be absolute or relative?

================
Comment at: clang-move/ClangMove.h:88
@@ +87,3 @@
+  // clang-move will change its current working directory to the build
+  // directory when analysising the source file. We saves the original working
+  // directory in order to get the absolute file path for the fields in Spec.
----------------
s/saves/save the/


https://reviews.llvm.org/D24922