Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
clang-move/ClangMove.cpp | ||
---|---|---|
33 ↗ | (On Diff #72613) | This doesn't seems to be good error handling... maybe also print an error message in case this fails in release binary. |
39 ↗ | (On Diff #72613) | There are two versions of this. Please add comments about the differences. |
43 ↗ | (On Diff #72613) | Same as above. Error handling. |
62 ↗ | (On Diff #72613) | To generalize the function, maybe also remove dots in AbsoluteFilePath? |
78 ↗ | (On Diff #72613) | Looks like MakeAbsolutePath(SM, SearchPath) can be done in addIncludes since you are also passing SM. |
109 ↗ | (On Diff #72613) | I think it would be easier to read if you put OriginalRunningDirectory before OldHeader . |
clang-move/ClangMove.h | ||
63 ↗ | (On Diff #72613) | This function could use more comments. What are those parameters and what are they for? |
64 ↗ | (On Diff #72613) | Should SearchPath be absolute or relative? |
88 ↗ | (On Diff #72613) | s/saves/save the/ |
Address review comments.
clang-move/ClangMove.cpp | ||
---|---|---|
62 ↗ | (On Diff #72613) | AbsoluteFilePath has been removed dots, and the matcher here is only used internally by clang-move, so I'd prefer the current way here, if we meet the generalized case in the future, I will be happy to extend it. |