[NFC][analyzer] Remove CheckerNameRef::getName() #130780
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`CheckerNameRef` is a trivial wrapper around a `StringRef` which is guaranteed to be owned by the `CheckerRegistry` (the only `friend` of the class) because other code can't call the private constructor. This class had offered two ways to recover the plain `StringRef`: an an `operator StringRef()` for implicit conversion and a method `StringRef getName()` which could be called explicitly. However this method name was really confusing, because it implies "get the name of this object" instead of "get this name as a plain `StringRef`"; so I removed it from the codebase and used `static_cast<StringRef>` in the two locations where the cast wasn't performed implicitly. This commit "prepares the ground" for planned improvements in checker name handling.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) Changes
This class had offered two ways to recover the plain However this method name was really confusing, because it implies "get the name of this object" instead of "get this name as a plain This commit "prepares the ground" for planned improvements in checker name handling. Full diff: https://github.com/llvm/llvm-project/pull/130780.diff 7 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index 7d3120b5395c4..929ce96824e95 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -113,7 +113,6 @@ class CheckerNameRef {
public:
CheckerNameRef() = default;
- StringRef getName() const { return Name; }
operator StringRef() const { return Name; }
};
diff --git a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
index 0b6c5163a1637..1b87a0d8af38d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
@@ -431,7 +431,7 @@ void ExprInspectionChecker::analyzerHashDump(const CallExpr *CE,
const SourceManager &SM = C.getSourceManager();
FullSourceLoc FL(CE->getArg(0)->getBeginLoc(), SM);
std::string HashContent =
- getIssueString(FL, getCheckerName().getName(), "Category",
+ getIssueString(FL, getCheckerName(), "Category",
C.getLocationContext()->getDecl(), Opts);
reportBug(HashContent, C);
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 0d1213ddf8b01..1c4293c30abdb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3885,7 +3885,7 @@ void MallocChecker::printState(raw_ostream &Out, ProgramStateRef State,
Out << " : ";
Data.dump(Out);
if (CheckKind)
- Out << " (" << CheckNames[*CheckKind].getName() << ")";
+ Out << " (" << CheckNames[*CheckKind] << ")";
Out << NL;
}
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
index 28320f46f237a..bd2f88c7b1bcc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -272,12 +272,12 @@ void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
if (!BT_leakedvalist) {
// FIXME: maybe creating a new check name for this type of bug is a better
// solution.
- BT_leakedvalist.reset(
- new BugType(CheckNames[CK_Unterminated].getName().empty()
- ? CheckNames[CK_Uninitialized]
- : CheckNames[CK_Unterminated],
- "Leaked va_list", categories::MemoryError,
- /*SuppressOnSink=*/true));
+ BT_leakedvalist.reset(new BugType(
+ static_cast<StringRef>(CheckNames[CK_Unterminated]).empty()
+ ? CheckNames[CK_Uninitialized]
+ : CheckNames[CK_Unterminated],
+ "Leaked va_list", categories::MemoryError,
+ /*SuppressOnSink=*/true));
}
const ExplodedNode *StartNode = getStartCallSite(N, Reg);
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index 7101b1eb2c7f5..93ea4cc1d66db 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3442,8 +3442,8 @@ void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
BugType *BugReporter::getBugTypeForName(CheckerNameRef CheckName,
StringRef name, StringRef category) {
SmallString<136> fullDesc;
- llvm::raw_svector_ostream(fullDesc) << CheckName.getName() << ":" << name
- << ":" << category;
+ llvm::raw_svector_ostream(fullDesc)
+ << CheckName << ":" << name << ":" << category;
std::unique_ptr<BugType> &BT = StrBugTypes[fullDesc];
if (!BT)
BT = std::make_unique<BugType>(CheckName, name, category);
diff --git a/clang/lib/StaticAnalyzer/Core/Checker.cpp b/clang/lib/StaticAnalyzer/Core/Checker.cpp
index bc1c8964b3ee4..317acdb8b2ddd 100644
--- a/clang/lib/StaticAnalyzer/Core/Checker.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Checker.cpp
@@ -18,9 +18,7 @@ using namespace ento;
int ImplicitNullDerefEvent::Tag;
-StringRef CheckerBase::getTagDescription() const {
- return getCheckerName().getName();
-}
+StringRef CheckerBase::getTagDescription() const { return getCheckerName(); }
CheckerNameRef CheckerBase::getCheckerName() const { return Name; }
@@ -30,10 +28,10 @@ CheckerProgramPointTag::CheckerProgramPointTag(StringRef CheckerName,
CheckerProgramPointTag::CheckerProgramPointTag(const CheckerBase *Checker,
StringRef Msg)
- : SimpleProgramPointTag(Checker->getCheckerName().getName(), Msg) {}
+ : SimpleProgramPointTag(Checker->getCheckerName(), Msg) {}
raw_ostream& clang::ento::operator<<(raw_ostream &Out,
const CheckerBase &Checker) {
- Out << Checker.getCheckerName().getName();
+ Out << Checker.getCheckerName();
return Out;
}
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index 53929d370e2fe..0e9b6bcc93e79 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 ? Checker->getCheckerName().getName() : "<unknown>";
+ Checker ? static_cast<StringRef>(Checker->getCheckerName()) : "<unknown>";
return (Name + ":" + CheckerName).str();
}
@@ -721,7 +721,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
"The '{0}' call has been already evaluated by the {1} checker, "
"while the {2} checker also tried to evaluate the same call. At "
"most one checker supposed to evaluate a call.",
- toString(Call), evaluatorChecker->getName(),
+ toString(Call), evaluatorChecker,
EvalCallChecker.Checker->getCheckerName());
llvm_unreachable(AssertionMessage.c_str());
}
@@ -799,7 +799,7 @@ void CheckerManager::runCheckersForPrintStateJson(raw_ostream &Out,
continue;
Indent(Out, Space, IsDot)
- << "{ \"checker\": \"" << CT.second->getCheckerName().getName()
+ << "{ \"checker\": \"" << CT.second->getCheckerName()
<< "\", \"messages\": [" << NL;
Indent(Out, InnerSpace, IsDot)
<< '\"' << TempBuf.str().trim() << '\"' << NL;
|
|
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) Changes
This class had offered two ways to recover the plain However this method name was really confusing, because it implies "get the name of this object" instead of "get this name as a plain This commit "prepares the ground" for planned improvements in checker name handling. Full diff: https://github.com/llvm/llvm-project/pull/130780.diff 7 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index 7d3120b5395c4..929ce96824e95 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -113,7 +113,6 @@ class CheckerNameRef {
public:
CheckerNameRef() = default;
- StringRef getName() const { return Name; }
operator StringRef() const { return Name; }
};
diff --git a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
index 0b6c5163a1637..1b87a0d8af38d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
@@ -431,7 +431,7 @@ void ExprInspectionChecker::analyzerHashDump(const CallExpr *CE,
const SourceManager &SM = C.getSourceManager();
FullSourceLoc FL(CE->getArg(0)->getBeginLoc(), SM);
std::string HashContent =
- getIssueString(FL, getCheckerName().getName(), "Category",
+ getIssueString(FL, getCheckerName(), "Category",
C.getLocationContext()->getDecl(), Opts);
reportBug(HashContent, C);
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 0d1213ddf8b01..1c4293c30abdb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3885,7 +3885,7 @@ void MallocChecker::printState(raw_ostream &Out, ProgramStateRef State,
Out << " : ";
Data.dump(Out);
if (CheckKind)
- Out << " (" << CheckNames[*CheckKind].getName() << ")";
+ Out << " (" << CheckNames[*CheckKind] << ")";
Out << NL;
}
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
index 28320f46f237a..bd2f88c7b1bcc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -272,12 +272,12 @@ void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
if (!BT_leakedvalist) {
// FIXME: maybe creating a new check name for this type of bug is a better
// solution.
- BT_leakedvalist.reset(
- new BugType(CheckNames[CK_Unterminated].getName().empty()
- ? CheckNames[CK_Uninitialized]
- : CheckNames[CK_Unterminated],
- "Leaked va_list", categories::MemoryError,
- /*SuppressOnSink=*/true));
+ BT_leakedvalist.reset(new BugType(
+ static_cast<StringRef>(CheckNames[CK_Unterminated]).empty()
+ ? CheckNames[CK_Uninitialized]
+ : CheckNames[CK_Unterminated],
+ "Leaked va_list", categories::MemoryError,
+ /*SuppressOnSink=*/true));
}
const ExplodedNode *StartNode = getStartCallSite(N, Reg);
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index 7101b1eb2c7f5..93ea4cc1d66db 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3442,8 +3442,8 @@ void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
BugType *BugReporter::getBugTypeForName(CheckerNameRef CheckName,
StringRef name, StringRef category) {
SmallString<136> fullDesc;
- llvm::raw_svector_ostream(fullDesc) << CheckName.getName() << ":" << name
- << ":" << category;
+ llvm::raw_svector_ostream(fullDesc)
+ << CheckName << ":" << name << ":" << category;
std::unique_ptr<BugType> &BT = StrBugTypes[fullDesc];
if (!BT)
BT = std::make_unique<BugType>(CheckName, name, category);
diff --git a/clang/lib/StaticAnalyzer/Core/Checker.cpp b/clang/lib/StaticAnalyzer/Core/Checker.cpp
index bc1c8964b3ee4..317acdb8b2ddd 100644
--- a/clang/lib/StaticAnalyzer/Core/Checker.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Checker.cpp
@@ -18,9 +18,7 @@ using namespace ento;
int ImplicitNullDerefEvent::Tag;
-StringRef CheckerBase::getTagDescription() const {
- return getCheckerName().getName();
-}
+StringRef CheckerBase::getTagDescription() const { return getCheckerName(); }
CheckerNameRef CheckerBase::getCheckerName() const { return Name; }
@@ -30,10 +28,10 @@ CheckerProgramPointTag::CheckerProgramPointTag(StringRef CheckerName,
CheckerProgramPointTag::CheckerProgramPointTag(const CheckerBase *Checker,
StringRef Msg)
- : SimpleProgramPointTag(Checker->getCheckerName().getName(), Msg) {}
+ : SimpleProgramPointTag(Checker->getCheckerName(), Msg) {}
raw_ostream& clang::ento::operator<<(raw_ostream &Out,
const CheckerBase &Checker) {
- Out << Checker.getCheckerName().getName();
+ Out << Checker.getCheckerName();
return Out;
}
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index 53929d370e2fe..0e9b6bcc93e79 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 ? Checker->getCheckerName().getName() : "<unknown>";
+ Checker ? static_cast<StringRef>(Checker->getCheckerName()) : "<unknown>";
return (Name + ":" + CheckerName).str();
}
@@ -721,7 +721,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
"The '{0}' call has been already evaluated by the {1} checker, "
"while the {2} checker also tried to evaluate the same call. At "
"most one checker supposed to evaluate a call.",
- toString(Call), evaluatorChecker->getName(),
+ toString(Call), evaluatorChecker,
EvalCallChecker.Checker->getCheckerName());
llvm_unreachable(AssertionMessage.c_str());
}
@@ -799,7 +799,7 @@ void CheckerManager::runCheckersForPrintStateJson(raw_ostream &Out,
continue;
Indent(Out, Space, IsDot)
- << "{ \"checker\": \"" << CT.second->getCheckerName().getName()
+ << "{ \"checker\": \"" << CT.second->getCheckerName()
<< "\", \"messages\": [" << NL;
Indent(Out, InnerSpace, IsDot)
<< '\"' << TempBuf.str().trim() << '\"' << NL;
|
|
🤔 Instead of this change I could also imagine introducing an accurately named explicit method – e.g. @steakhal or anybody else: What do you think about this situation? Which would be the more elegant approach: the implicit I would be grateful for a quick review on this PR, because I have several mostly written changes that I will need to refactor depending on this change. |
Xazax-hun
approved these changes
Mar 11, 2025
|
I have no strong feelings about the implicit/explicit question. I am also not sure we need this lightweight wrapper type. |
I also fail to see the benefit of this strong-type. |
steakhal
approved these changes
Mar 11, 2025
|
I agree that removing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CheckerNameRefis a trivial wrapper around aStringRefwhich is guaranteed to be owned by theCheckerRegistry(the onlyfriendof the class) because other code can't call the private constructor.This class had offered two ways to recover the plain
StringRef: an anoperator StringRef()for implicit conversion and a methodStringRef getName()which could be called explicitly.However this method name was really confusing, because it implies "get the name of this object" instead of "get this name as a plain
StringRef"; so I removed it from the codebase and usedstatic_cast<StringRef>in the two locations where the cast wasn't performed implicitly.This commit "prepares the ground" for planned improvements in checker name handling.