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. |