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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
