[PATCH] D23651: [clang-rename] improve performance for rename-all

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 18 08:47:33 PDT 2016


alexfh requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-rename/USRFindingAction.cpp:69
@@ -69,2 +68,3 @@
     }
-    USRs->insert(USRs->end(), USRSet.begin(), USRSet.end());
+    USRs.insert(USRs.end(), USRSet.begin(), USRSet.end());
+    return USRs;
----------------
Should USRs be a local variable now?

================
Comment at: clang-rename/USRFindingAction.cpp:147
@@ +146,3 @@
+  explicit NamedDeclFindingConsumer(
+      const std::vector<unsigned> &SymbolOffsets,
+      const std::vector<std::string> &OldNames,
----------------
Use `ArrayRef` here as well. BTW, if the code relies on `SymbolOffsets` and `OldNames` being of the same length, maybe a single collection of pairs would work better? Or define a structure for keeping offset and old name together?

================
Comment at: clang-rename/USRFindingAction.h:29
@@ -27,3 +28,3 @@
 struct USRFindingAction {
-  USRFindingAction(unsigned Offset, const std::string &Name)
-      : SymbolOffset(Offset), OldName(Name) {}
+  USRFindingAction(const std::vector<unsigned> &SymbolOffsets,
+                   const std::vector<std::string> &OldNames)
----------------
Use `ArrayRef` instead of `const vector<>&`. `ArrayRef<>` is less restrictive.

================
Comment at: clang-rename/USRFindingAction.h:30
@@ +29,3 @@
+  USRFindingAction(const std::vector<unsigned> &SymbolOffsets,
+                   const std::vector<std::string> &OldNames)
+      : SymbolOffsets(SymbolOffsets), OldNames(OldNames) {}
----------------
Use `ArrayRef<std::string>`.

================
Comment at: clang-rename/USRFindingAction.h:38
@@ -37,5 +37,3 @@
 private:
-  unsigned SymbolOffset;
-  std::string OldName;
-  std::string SpellingName;
-  std::vector<std::string> USRs;
+  const std::vector<unsigned> &SymbolOffsets;
+  const std::vector<std::string> &OldNames;
----------------
omtcyfz wrote:
> Aw, you're right. Good catch, thanks!
Reference members always seem suspicious to me. One has to be really really careful not to mess up lifetimes. Are we actually saving much but not copying these vectors?


https://reviews.llvm.org/D23651





More information about the cfe-commits mailing list