hokein added a comment. Sorry for the delay. The code looks good roughly, a few nits below.
================ Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:19 +namespace clang { +namespace tooling { + ---------------- An off-topic thought: currently we put everything into `clang::tooling`, I think we might need a separate namespace e.g. `clang::tooling::refactoring` in the future? ================ Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:21 + +class SymbolName; + ---------------- I think this can be removed? ================ Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:60 +static void +applyChanges(ArrayRef<AtomicChange> AtomicChanges, + std::map<std::string, tooling::Replacements> &FileToReplaces) { ---------------- Maybe add a comment for this utility function. The name is a bit confusing, IIUC, the function actually fills the `FileToReplaces`, not apply the changes? maybe a better name for it? ================ Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:61 +applyChanges(ArrayRef<AtomicChange> AtomicChanges, + std::map<std::string, tooling::Replacements> &FileToReplaces) { + for (const auto AtomicChange : AtomicChanges) { ---------------- nit: For out parameter, I'd use pointer. ================ Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:62 + std::map<std::string, tooling::Replacements> &FileToReplaces) { + for (const auto AtomicChange : AtomicChanges) { + for (const auto &Replace : AtomicChange.getReplacements()) { ---------------- nit: `const auto &` ================ Comment at: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp:17 + +SymbolOccurrence::SymbolOccurrence(const SymbolName &Name, OccurrenceKind Kind, + ArrayRef<SourceLocation> Locations) ---------------- Shouldn't the definition be in clang::tooling namespace? ================ Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:398 Visitor.TraverseDecl(Decl); - return Visitor.getLocationsFound(); + return std::move(Visitor.getOccurrences()); } ---------------- I think just returning `Visitor.getOccurrences()` is sufficient -- compiler can handle it well, also using std::move would prevent copy elision. Repository: rL LLVM https://reviews.llvm.org/D36156 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits