[PATCH] D38402: [clang-refactor] Apply source replacements

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: include/clang/Frontend/CommandLineSourceLoc.h:57
+  std::string FileName;
+  std::pair<unsigned, unsigned> Begin;
+  std::pair<unsigned, unsigned> End;
----------------
Add a comment documenting what the first element and second element of the pair represent for (<Line, Column>? If I understand correctly).


================
Comment at: include/clang/Frontend/CommandLineSourceLoc.h:60
+
+  /// Returns a parsed source range from a string or None if the string is
+  /// invalid.
----------------
Would be clearer to document the valid format of the `Str`. 


================
Comment at: test/Refactor/tool-apply-replacements.cpp:3
+// RUN: cp %s %t.cp.cpp
+// RUN: clang-refactor local-rename -selection=%t.cp.cpp:6:7 -new-name=test %t.cp.cpp --
+// RUN: grep -v CHECK %t.cp.cpp | FileCheck %t.cp.cpp
----------------
Maybe add one more case for testing `-selection=%t.cp.cpp:6:7-6:15`.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:309
 public:
   void handleError(llvm::Error Err) {
     llvm::errs() << llvm::toString(std::move(Err)) << "\n";
----------------
Add `override`.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:313
 
-  // FIXME: Consume atomic changes and apply them to files.
+  void handle(AtomicChanges Changes) {
+    SourceChanges.insert(SourceChanges.begin(), Changes.begin(), Changes.end());
----------------
The same, `override`.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:412
+      if (!BufferErr) {
+        llvm::errs() << "error: failed to open" << File << " for rewriting\n";
+        return true;
----------------
nit: missing a blank after `open`.


Repository:
  rL LLVM

https://reviews.llvm.org/D38402