[NFC][analyzer] Rename CheckerBase::getCheckerName to getName

[NagyDonat]

The method name getCheckerName would imply "get the name of the checker associated with this", so it's suitable for e.g. BugType::getCheckerName -- but a method that just "gets the name of this" should be simply called getName.

This change eliminates the redundant and ugly pattern Checker->getCheckerName() and helps to visually distinguish this method from BugType::getCheckerName.

In the constructor of BugType the call of this method was completely removed (instead of just changing the name) because that call was dead code (when the data member Checker is non-null, the string stored in CheckerName is irrelevant) and was often querying the name of the checker before it was properly initialized.

Moreover, in ReturnValueChecker.cpp the static helper function getName (which gets a function name from a CallEvent) was renamed to the more specific getFunctionName to avoid the name collision.

This change is yet another cleanup commit before my planned changes that would add support for multi-part checkers to this method.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

The method name getCheckerName would imply "get the name of the checker associated with this", so it's suitable for e.g. BugType::getCheckerName -- but the proper name for a method that "gets the name of this" should be simply called getName.

This change eliminates the redundant and ugly pattern Checker->getCheckerName() and helps to visually distinguish this method from BugType::getCheckerName.

In the constructor of BugType the call of this method was completely removed (instead of just changing the name) because that call was dead code (when the data member Checker is non-null, the string stored in CheckerName is irrelevant) and was often querying the name of the checker before it was properly initialized.

Moreover, in ReturnValueChecker.cpp the static helper function getName (which gets a function name from a CallEvent) was renamed to the more specific getFunctionName to avoid the name collision.

This change is yet another cleanup commit before my planned changes that would add support for multi-part checkers to this method.

---

Full diff: https://github.com/llvm/llvm-project/pull/130953.diff

7 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h (+4-4)
• (modified) clang/include/clang/StaticAnalyzer/Core/Checker.h (+1-1)
• (modified) clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp (+2-3)
• (modified) clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp (+3-3)
• (modified) clang/lib/StaticAnalyzer/Core/BugReporter.cpp (+2-2)
• (modified) clang/lib/StaticAnalyzer/Core/Checker.cpp (+4-4)
• (modified) clang/lib/StaticAnalyzer/Core/CheckerManager.cpp (+5-6)

diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
index e50afd6d0da7e..97cf7eda0ae2f 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
@@ -41,19 +41,19 @@ class BugType {
         Checker(nullptr), SuppressOnSink(SuppressOnSink) {}
   BugType(const CheckerBase *Checker, StringRef Desc,
           StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
-      : CheckerName(Checker->getCheckerName()), Description(Desc),
-        Category(Cat), Checker(Checker), SuppressOnSink(SuppressOnSink) {}
+      : CheckerName(), Description(Desc), Category(Cat), Checker(Checker),
+        SuppressOnSink(SuppressOnSink) {}
   virtual ~BugType() = default;
 
   StringRef getDescription() const { return Description; }
   StringRef getCategory() const { return Category; }
   StringRef getCheckerName() const {
-    // FIXME: This is a workaround to ensure that the correct checerk name is
+    // FIXME: This is a workaround to ensure that the correct checker name is
     // used. The checker names are set after the constructors are run.
     // In case the BugType object is initialized in the checker's ctor
     // the CheckerName field will be empty. To circumvent this problem we use
     // CheckerBase whenever it is possible.
-    StringRef Ret = Checker ? Checker->getCheckerName() : CheckerName;
+    StringRef Ret = Checker ? Checker->getName() : CheckerName;
     assert(!Ret.empty() && "Checker name is not set properly.");
     return Ret;
   }
diff --git a/clang/include/clang/StaticAnalyzer/Core/Checker.h b/clang/include/clang/StaticAnalyzer/Core/Checker.h
index 2ec54a837c42c..4d9b33cc559b8 100644
--- a/clang/include/clang/StaticAnalyzer/Core/Checker.h
+++ b/clang/include/clang/StaticAnalyzer/Core/Checker.h
@@ -490,7 +490,7 @@ class CheckerBase : public ProgramPointTag {
 
 public:
   StringRef getTagDescription() const override;
-  CheckerNameRef getCheckerName() const;
+  CheckerNameRef getName() const;
 
   /// See CheckerManager::runCheckersForPrintState.
   virtual void printState(raw_ostream &Out, ProgramStateRef State,
diff --git a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
index 1b87a0d8af38d..6035e2d34c2b3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
@@ -430,9 +430,8 @@ void ExprInspectionChecker::analyzerHashDump(const CallExpr *CE,
   const LangOptions &Opts = C.getLangOpts();
   const SourceManager &SM = C.getSourceManager();
   FullSourceLoc FL(CE->getArg(0)->getBeginLoc(), SM);
-  std::string HashContent =
-      getIssueString(FL, getCheckerName(), "Category",
-                     C.getLocationContext()->getDecl(), Opts);
+  std::string HashContent = getIssueString(
+      FL, getName(), "Category", C.getLocationContext()->getDecl(), Opts);
 
   reportBug(HashContent, C);
 }
diff --git a/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
index 3da571adfa44c..1e114149debf5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
@@ -60,7 +60,7 @@ class ReturnValueChecker : public Checker<check::PostCall> {
 };
 } // namespace
 
-static std::string getName(const CallEvent &Call) {
+static std::string getFunctionName(const CallEvent &Call) {
   std::string Name;
   if (const auto *MD = dyn_cast<CXXMethodDecl>(Call.getDecl()))
     if (const CXXRecordDecl *RD = MD->getParent())
@@ -84,7 +84,7 @@ void ReturnValueChecker::checkPostCall(const CallEvent &Call,
   if (ProgramStateRef StTrue = State->assume(*ReturnV, true)) {
     // The return value can be true, so transition to a state where it's true.
     std::string Msg =
-        formatv("'{0}' returns true (by convention)", getName(Call));
+        formatv("'{0}' returns true (by convention)", getFunctionName(Call));
     C.addTransition(StTrue, C.getNoteTag(Msg, /*IsPrunable=*/true));
     return;
   }
@@ -94,7 +94,7 @@ void ReturnValueChecker::checkPostCall(const CallEvent &Call,
   // Note that this checker is 'hidden' so it cannot produce a bug report.
   std::string Msg = formatv("'{0}' returned false, breaking the convention "
                             "that it always returns true",
-                            getName(Call));
+                            getFunctionName(Call));
   C.addTransition(State, C.getNoteTag(Msg, /*IsPrunable=*/true));
 }
 
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index 93ea4cc1d66db..a4f9e092e8205 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3418,8 +3418,8 @@ void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
                                   PathDiagnosticLocation Loc,
                                   ArrayRef<SourceRange> Ranges,
                                   ArrayRef<FixItHint> Fixits) {
-  EmitBasicReport(DeclWithIssue, Checker->getCheckerName(), Name, Category, Str,
-                  Loc, Ranges, Fixits);
+  EmitBasicReport(DeclWithIssue, Checker->getName(), Name, Category, Str, Loc,
+                  Ranges, Fixits);
 }
 
 void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
diff --git a/clang/lib/StaticAnalyzer/Core/Checker.cpp b/clang/lib/StaticAnalyzer/Core/Checker.cpp
index 317acdb8b2ddd..d93db6ddfc3bf 100644
--- a/clang/lib/StaticAnalyzer/Core/Checker.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Checker.cpp
@@ -18,9 +18,9 @@ using namespace ento;
 
 int ImplicitNullDerefEvent::Tag;
 
-StringRef CheckerBase::getTagDescription() const { return getCheckerName(); }
+StringRef CheckerBase::getTagDescription() const { return getName(); }
 
-CheckerNameRef CheckerBase::getCheckerName() const { return Name; }
+CheckerNameRef CheckerBase::getName() const { return Name; }
 
 CheckerProgramPointTag::CheckerProgramPointTag(StringRef CheckerName,
                                                StringRef Msg)
@@ -28,10 +28,10 @@ CheckerProgramPointTag::CheckerProgramPointTag(StringRef CheckerName,
 
 CheckerProgramPointTag::CheckerProgramPointTag(const CheckerBase *Checker,
                                                StringRef Msg)
-    : SimpleProgramPointTag(Checker->getCheckerName(), Msg) {}
+    : SimpleProgramPointTag(Checker->getName(), Msg) {}
 
 raw_ostream& clang::ento::operator<<(raw_ostream &Out,
                                      const CheckerBase &Checker) {
-  Out << Checker.getCheckerName();
+  Out << Checker.getName();
   return Out;
 }
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index 0e9b6bcc93e79..56b1e44acc1fd 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -139,7 +139,7 @@ std::string checkerScopeName(StringRef Name, const CheckerBase *Checker) {
   if (!llvm::timeTraceProfilerEnabled())
     return "";
   StringRef CheckerName =
-      Checker ? static_cast<StringRef>(Checker->getCheckerName()) : "<unknown>";
+      Checker ? static_cast<StringRef>(Checker->getName()) : "<unknown>";
   return (Name + ":" + CheckerName).str();
 }
 
@@ -722,12 +722,12 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
             "while the {2} checker also tried to evaluate the same call. At "
             "most one checker supposed to evaluate a call.",
             toString(Call), evaluatorChecker,
-            EvalCallChecker.Checker->getCheckerName());
+            EvalCallChecker.Checker->getName());
         llvm_unreachable(AssertionMessage.c_str());
       }
 #endif
       if (evaluated) {
-        evaluatorChecker = EvalCallChecker.Checker->getCheckerName();
+        evaluatorChecker = EvalCallChecker.Checker->getName();
         Dst.insert(checkDst);
 #ifdef NDEBUG
         break; // on release don't check that no other checker also evals.
@@ -798,9 +798,8 @@ void CheckerManager::runCheckersForPrintStateJson(raw_ostream &Out,
     if (TempBuf.empty())
       continue;
 
-    Indent(Out, Space, IsDot)
-        << "{ \"checker\": \"" << CT.second->getCheckerName()
-        << "\", \"messages\": [" << NL;
+    Indent(Out, Space, IsDot) << "{ \"checker\": \"" << CT.second->getName()
+                              << "\", \"messages\": [" << NL;
     Indent(Out, InnerSpace, IsDot)
         << '\"' << TempBuf.str().trim() << '\"' << NL;
     Indent(Out, Space, IsDot) << "]}";

[Xazax-hun]

Is Checker ever null? Do we still need the CheckerName field?

[NagyDonat]

Under the current system we still need CheckerName, but we will be able to remove it when I implement my new multipart checker framework and all multipart checkers are transferred to it.

Right now both constructors of BugType are used and needed:

• In simple straightforward checkers Checker->getCheckerName() can return the (single) name of the checker, so we can directly initialize the bug types as BugType BT{this, ...} with the constructor which takes CheckerBase *Checker and default initializes CheckerName.
• In multipart checkers the BugType currently cannot query the name of the relevant checker part from a CheckerBase * (because each multipart checker implements its own custom array of checker names), so we need to initialize the BugType with a CheckerNameRef (and set Checker to null), and this requires lazy initialization (the ugly mutable std::unique_ptr<BugType>) because the names of the sub-checkers are not yet available when the checker object is constructed.

The main effect of my multipart checker change is (roughly speaking) that I will provide a CheckerBase::getName(unsigned CheckerPartIdx) and then BugTypes can exactly reference a checker part with a pair of a CheckerBase* (the singleton instance of the checker class) and a numerical index (which identifies the part). After this, we can directly initialize each bug type object with either a CheckerBase* (simple checkers) or a <CheckerBase*, unsigned> pair (multipart checkers), so we can get rid of the BugType constructor that takes a raw CheckerName (and needs the awkward lazy initialization).