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