[clang-tools-extra] 8fdedcd - [clang-tidy] Optimize misc-confusable-identifiers

[Piotr Zegar via cfe-commits]

Author: Piotr Zegar
Date: 2023-06-10T11:06:49Z
New Revision: 8fdedcd1a242f6b54eec969e72e35ac0a68b7ea1

URL: https://github.com/llvm/llvm-project/commit/8fdedcd1a242f6b54eec969e72e35ac0a68b7ea1
DIFF: https://github.com/llvm/llvm-project/commit/8fdedcd1a242f6b54eec969e72e35ac0a68b7ea1.diff

LOG: [clang-tidy] Optimize misc-confusable-identifiers

This is final optimization for this check. Main
improvements comes from changing a logic order
in mayShadow function, to first validate result
of mayShadowImpl, then search primary context in
a vectors. Secondary improvement comes from excluding
all implicit code by using TK_IgnoreUnlessSpelledInSource.
All other changes are just cosmetic improvements.

Tested on Cataclysm-DDA open source project, result in
check execution time reduction from 3682 seconds to
100 seconds (~0.25s per TU). That's 97.2% reduction for
this change alone. Resulting in cumulative improvement for
this check around -99.6%, finally bringing this check
into a cheap category.

Reviewed By: serge-sans-paille

Differential Revision: https://reviews.llvm.org/D151594

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp
    clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.h

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp b/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp
index 63ba663aaca9a..c2f72c8fc20a0 100644
--- a/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp
@@ -11,6 +11,7 @@
 
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/Support/ConvertUTF.h"
 
 namespace {
@@ -45,14 +46,13 @@ ConfusableIdentifierCheck::~ConfusableIdentifierCheck() = default;
 // We're skipping 1. and 3. for the sake of simplicity, but this can lead to
 // false positive.
 
-static std::string skeleton(StringRef Name) {
+static llvm::SmallString<64U> skeleton(StringRef Name) {
   using namespace llvm;
-  std::string SName = Name.str();
-  std::string Skeleton;
-  Skeleton.reserve(1 + Name.size());
+  SmallString<64U> Skeleton;
+  Skeleton.reserve(1U + Name.size());
 
-  const char *Curr = SName.c_str();
-  const char *End = Curr + SName.size();
+  const char *Curr = Name.data();
+  const char *End = Curr + Name.size();
   while (Curr < End) {
 
     const char *Prev = Curr;
@@ -99,8 +99,6 @@ static bool mayShadowImpl(const NamedDecl *ND0, const NamedDecl *ND1) {
 
 static bool isMemberOf(const ConfusableIdentifierCheck::ContextInfo *DC0,
                        const ConfusableIdentifierCheck::ContextInfo *DC1) {
-  if (DC0->Bases.empty())
-    return false;
   return llvm::is_contained(DC1->Bases, DC0->PrimaryContext);
 }
 
@@ -117,16 +115,23 @@ static bool mayShadow(const NamedDecl *ND0,
                       const ConfusableIdentifierCheck::ContextInfo *DC0,
                       const NamedDecl *ND1,
                       const ConfusableIdentifierCheck::ContextInfo *DC1) {
-  if (!DC0->Bases.empty() && ND1->getAccess() != AS_private &&
-      isMemberOf(DC1, DC0))
-    return true;
-  if (!DC1->Bases.empty() && ND0->getAccess() != AS_private &&
-      isMemberOf(DC0, DC1))
-    return true;
 
-  return enclosesContext(DC0, DC1) &&
-         (mayShadowImpl(ND0, ND1) || mayShadowImpl(DC0->NonTransparentContext,
-                                                   DC1->NonTransparentContext));
+  if (!DC0->Bases.empty() && !DC1->Bases.empty()) {
+    // if any of the declaration is a non-private member of the other
+    // declaration, it's shadowed by the former
+
+    if (ND1->getAccess() != AS_private && isMemberOf(DC1, DC0))
+      return true;
+
+    if (ND0->getAccess() != AS_private && isMemberOf(DC0, DC1))
+      return true;
+  }
+
+  if (!mayShadowImpl(DC0->NonTransparentContext, DC1->NonTransparentContext) &&
+      !mayShadowImpl(ND0, ND1))
+    return false;
+
+  return enclosesContext(DC0, DC1);
 }
 
 const ConfusableIdentifierCheck::ContextInfo *
@@ -172,26 +177,41 @@ ConfusableIdentifierCheck::getContextInfo(const DeclContext *DC) {
 
 void ConfusableIdentifierCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
-  if (const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("nameddecl")) {
-    if (IdentifierInfo *NDII = ND->getIdentifier()) {
-      const ContextInfo *Info = getContextInfo(ND->getDeclContext());
-      StringRef NDName = NDII->getName();
-      llvm::SmallVector<Entry> &Mapped = Mapper[skeleton(NDName)];
-      for (const Entry &E : Mapped) {
-        const IdentifierInfo *ONDII = E.Declaration->getIdentifier();
-        if (mayShadow(ND, Info, E.Declaration, E.Info)) {
-          StringRef ONDName = ONDII->getName();
-          if (ONDName != NDName) {
-            diag(ND->getLocation(), "%0 is confusable with %1")
-                << ND << E.Declaration;
-            diag(E.Declaration->getLocation(), "other declaration found here",
-                 DiagnosticIDs::Note);
-          }
-        }
-      }
-      Mapped.push_back({ND, Info});
-    }
+  const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("nameddecl");
+  if (!ND)
+    return;
+
+  IdentifierInfo *NDII = ND->getIdentifier();
+  if (!NDII)
+    return;
+
+  StringRef NDName = NDII->getName();
+  if (NDName.empty())
+    return;
+
+  const ContextInfo *Info = getContextInfo(ND->getDeclContext());
+
+  llvm::SmallVector<Entry> &Mapped = Mapper[skeleton(NDName)];
+  for (const Entry &E : Mapped) {
+    if (!mayShadow(ND, Info, E.Declaration, E.Info))
+      continue;
+
+    const IdentifierInfo *ONDII = E.Declaration->getIdentifier();
+    StringRef ONDName = ONDII->getName();
+    if (ONDName == NDName)
+      continue;
+
+    diag(ND->getLocation(), "%0 is confusable with %1") << ND << E.Declaration;
+    diag(E.Declaration->getLocation(), "other declaration found here",
+         DiagnosticIDs::Note);
   }
+
+  Mapped.push_back({ND, Info});
+}
+
+void ConfusableIdentifierCheck::onEndOfTranslationUnit() {
+  Mapper.clear();
+  ContextInfos.clear();
 }
 
 void ConfusableIdentifierCheck::registerMatchers(

diff  --git a/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.h b/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.h
index ede058c7197b7..f3b0c8ed00306 100644
--- a/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.h
@@ -26,6 +26,10 @@ class ConfusableIdentifierCheck : public ClangTidyCheck {
 
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void onEndOfTranslationUnit() override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 
   struct ContextInfo {
     const DeclContext *PrimaryContext;