[PATCH] D149723: [clang-tidy] Optimize performance of RenamerClangTidyCheck

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked 3 inline comments as done.
PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:47
 
-    std::hash<NamingCheckId::second_type> SecondHash;
-    return DenseMapInfo<clang::SourceLocation>::getHashValue(Val.first) +
-           SecondHash(Val.second);
+    return hash_combine(
+        DenseMapInfo<clang::SourceLocation>::getHashValue(Val.first),
----------------
njames93 wrote:
> What's the purpose of using hash_combine instead of just adding the 2 hashes together, there is enough entropy in the hashes that a simple operation makes more sense?
Habit, hash_combine sounded better here, I can change this to +.


================
Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:201
 
-    addUsage(Decl->getParent(), Decl->getNameInfo().getSourceRange(),
-             Result.SourceManager);
+  bool shouldVisitTemplateInstantiations() const { return true; }
+
----------------
njames93 wrote:
> What is the reason for visiting template instantiations?
Tests were failing, some method overrides are known only when we visit template instances. Without this code related to "Fix overridden methods"  (line 281) were not executed properly.
And as an result overridden virtual methods in template classes were not "fixed". Previously it worked in same way.


================
Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:495
+                                  AggressiveDependentMemberLookup);
+  Visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(Decl));
 }
----------------
njames93 wrote:
> Instead of this nasty const cast, you can just use `Visitor.TraverseAST(*Result.Context);`
Yes I know, but it looked strange when we register matcher for something, and then in check we don't use that something.
I can change this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149723/new/

https://reviews.llvm.org/D149723