[PATCH] D22408: [clang-rename] add support for overridden functions

Manuel Klimek via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 18 00:34:35 PDT 2016


klimek added inline comments.

================
Comment at: clang-rename/USRFindingAction.cpp:12
@@ -12,1 +11,3 @@
+/// \brief Provides an action to find USR for the symbol at <offset> and all
+/// relevant USRs aswell.
 ///
----------------
... at <offset>, as well as all relevant USRs.

================
Comment at: clang-rename/USRFindingAction.cpp:60
@@ +59,3 @@
+  bool VisitCXXMethodDecl(CXXMethodDecl *D) {
+    if (D->isVirtual()) {
+      bool Found = false;
----------------
I'd use early exit here:
if (!D->isVirtual) return true;
...

================
Comment at: clang-rename/USRFindingAction.cpp:62
@@ +61,3 @@
+      bool Found = false;
+      for (const auto &OverriddenMethod : D->overridden_methods()) {
+        if (std::find(USRs->begin(), USRs->end(),
----------------
This is n^3, right? (for each virtual method we run over all overrides, searching through all USRs)
a) would be cool if we could use matchers for this, as they automatically memoize; if we don't want to do that, we might want to memoize ourselves
b) sorting USRs and using binary_search should be a cheap speed-up

================
Comment at: clang-rename/USRFindingAction.cpp:144
@@ -101,1 +143,3 @@
+    auto *TUDecl = Context.getTranslationUnitDecl();
+    RelevantUSRFinder Finder(FoundDecl, TUDecl, USRs);
   }
----------------
Having all side-effects in the constructor is really unexpected to me. I'd add a Find() method.


https://reviews.llvm.org/D22408





More information about the cfe-commits mailing list