arphaman marked 5 inline comments as done. arphaman added inline comments.
================ Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:19 +namespace clang { +namespace tooling { + ---------------- hokein wrote: > 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? That would be nice, I agree. Don't think it's in scope for this patch though, maybe for https://reviews.llvm.org/D36075? ================ Comment at: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp:17 + +SymbolOccurrence::SymbolOccurrence(const SymbolName &Name, OccurrenceKind Kind, + ArrayRef<SourceLocation> Locations) ---------------- hokein wrote: > Shouldn't the definition be in clang::tooling namespace? That's not necessary because of the `using namespace` directives above. Only standalone functions have to be defined in namespaces, out-of-line member function definitions don't have to follow that rule because they already have the class qualifier. ================ Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:398 Visitor.TraverseDecl(Decl); - return Visitor.getLocationsFound(); + return std::move(Visitor.getOccurrences()); } ---------------- hokein wrote: > I think just returning `Visitor.getOccurrences()` is sufficient -- compiler > can handle it well, also using std::move would prevent copy elision. I'm returning by non-const reference from `getOccurrences`, so the compiler copies by default. I have to move either here or return by value from `getOccurrences` and move there. We also have warnings for redundant `std::move`, and they don't fire here. 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