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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
