[PATCH] D23198: clang-rename rename-all: support reading old/newname pairs from a YAML file

[Alexander Kornienko via cfe-commits]

alexfh added a subscriber: alexfh.
alexfh added a comment.

A few late comments.


================
Comment at: clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp:65
@@ +64,3 @@
+
+  RenameAllInfo() : Offset(0) {}
+};
----------------
I'd use inline inititalizer instead.

================
Comment at: clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp:73
@@ +72,3 @@
+
+/// \brief Specialized MappingTraits to describe how a RenameAllInfo is /
+/// (de)serialized.
----------------
Remove the stray slash.

================
Comment at: clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp:152
@@ +151,3 @@
+    // Populate OldNames and NewNames from a YAML file.
+    auto Buffer = llvm::MemoryBuffer::getFile(Input);
+    if (!Buffer) {
----------------
Please use the explicit type, since it's not clear from the context.

================
Comment at: clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp:156
@@ +155,3 @@
+             << Buffer.getError().message() << "\n";
+      exit(1);
+    }
----------------
Instead, I'd return the error code. The return value of this function seems to be propagated to the main's return value anyway.

================
Comment at: clang-tools-extra/trunk/docs/clang-rename.rst:45
@@ -44,1 +44,3 @@
 
+The tool currently supports renaming actions inside a single Translation Unit
+only. It is planned to extend the tool's functionality to support multi-TU
----------------
"Translation unit" is not a proper name, it shouldn't be capitalized.


Repository:
  rL LLVM

https://reviews.llvm.org/D23198