[PATCH] D36156: [rename] Introduce symbol occurrences

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 14 05:56:21 PDT 2017


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





More information about the cfe-commits mailing list