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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: test/Refactor/tool-apply-replacements.cpp:6
+
+class RenameMe { // CHECK: class {
+};
----------------
Why is the result `class {`?


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:319
 
-  // FIXME: Consume atomic changes and apply them to files.
+  void handle(AtomicChanges SourceReplacements) {
+    AllSourceReplacements.insert(AllSourceReplacements.begin(),
----------------
I think `Changes` or `SourceChanges` would be a clearer variable name.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:330
+private:
+  AtomicChanges AllSourceReplacements;
 };
----------------
s/AllSourceReplacements/Changes or SourceChanges for clarity.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:410
 
+  bool applySourceReplacements(const AtomicChanges &Replacements) {
+    std::set<std::string> Files;
----------------
s/Replacements/Changes/ ?


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:416
+    tooling::ApplyChangesSpec Spec;
+    Spec.Cleanup = false;
+    for (const auto &File : Files) {
----------------
I think we would also want to clean up by default in the future. Maybe add this to FIXME as well?


Repository:
  rL LLVM

https://reviews.llvm.org/D38402