[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
=?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: https://github.com/NagyDonat closed https://github.com/llvm/llvm-project/pull/86953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
=?utf-8?q?Donát?= Nagy
Message-ID:
In-Reply-To:
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/86953
>From 638497ef227cf6f3e8d558618a186a3c45935dfc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Thu, 28 Mar 2024 14:49:15 +0100
Subject: [PATCH 1/2] [analyzer] Remove barely used class 'KnownSVal' (NFC)
The class `KnownSVal` was very magical abstract class within the `SVal`
class hierarchy: with a hacky `classof` method it acted as if it was the
common ancestor of the classes `UndefinedSVal` and `DefinedSVal`.
However, it was only used in two `getAs()` calls and the
signatures of two methods, which does not "pay for" its weird behavior,
so I created this commit that removes it and replaces its use with more
straightforward solutions.
---
.../Core/BugReporter/BugReporterVisitors.h | 3 ++-
.../StaticAnalyzer/Core/PathSensitive/SVals.h | 8
.../RetainCountChecker/RetainCountDiagnostics.cpp | 2 +-
.../StaticAnalyzer/Core/BugReporterVisitors.cpp| 14 +++---
4 files changed, 10 insertions(+), 17 deletions(-)
diff --git
a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index d9b3d9352d3224..cc3d93aabafda4 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -374,6 +374,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr
*E,
/// from.
///
/// \param V We're searching for the store where \c R received this value.
+///It may be either defined or undefined, but should not be unknown.
/// \param R The region we're tracking.
/// \param Opts Tracking options specifying how we want to track the value.
/// \param Origin Only adds notes when the last store happened in a
@@ -383,7 +384,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr
*E,
///changes to its value in a nested stackframe could be pruned, and
///this visitor can prevent that without polluting the bugpath too
///much.
-void trackStoredValue(KnownSVal V, const MemRegion *R,
+void trackStoredValue(SVal V, const MemRegion *R,
PathSensitiveBugReport , TrackingOptions Opts =
{},
const StackFrameContext *Origin = nullptr);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
index c60528b7685fe8..3a4b0872571494 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -232,14 +232,6 @@ class DefinedSVal : public DefinedOrUnknownSVal {
: DefinedOrUnknownSVal(Kind, Data) {}
};
-/// Represents an SVal that is guaranteed to not be UnknownVal.
-class KnownSVal : public SVal {
-public:
- /*implicit*/ KnownSVal(DefinedSVal V) : SVal(V) {}
- /*implicit*/ KnownSVal(UndefinedVal V) : SVal(V) {}
- static bool classof(SVal V) { return !V.isUnknown(); }
-};
-
class NonLoc : public DefinedSVal {
protected:
NonLoc(SValKind Kind, const void *Data) : DefinedSVal(Kind, Data) {}
diff --git
a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
index c3acb73ba7175b..086c3e5e49b770 100644
---
a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -977,7 +977,7 @@ void RefLeakReport::findBindingToReport(CheckerContext ,
// something like derived regions if we want to construct SVal from
// Sym. Instead, we take the value that is definitely stored in that
// region, thus guaranteeing that trackStoredValue will work.
-bugreporter::trackStoredValue(AllVarBindings[0].second.castAs(),
+bugreporter::trackStoredValue(AllVarBindings[0].second,
AllocBindingToReport, *this);
} else {
AllocBindingToReport = AllocFirstBinding;
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index a0822513a6d02e..ce167d979d5761 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1238,12 +1238,12 @@ class StoreSiteFinder final : public
TrackingBugReporterVisitor {
///changes to its value in a nested stackframe could be pruned, and
///this visitor can prevent that without polluting the bugpath too
///much.
- StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
+ StoreSiteFinder(bugreporter::TrackerRef ParentTracker, SVal V,
const MemRegion *R, TrackingOptions Options,
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/86953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
@@ -1238,12 +1238,12 @@ class StoreSiteFinder final : public
TrackingBugReporterVisitor {
///changes to its value in a nested stackframe could be pruned, and
///this visitor can prevent that without polluting the bugpath too
///much.
- StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
+ StoreSiteFinder(bugreporter::TrackerRef ParentTracker, SVal V,
const MemRegion *R, TrackingOptions Options,
const StackFrameContext *OriginSFC = nullptr)
: TrackingBugReporterVisitor(ParentTracker), R(R), V(V),
Options(Options),
OriginSFC(OriginSFC) {
-assert(R);
+assert(!V.isUnknown() && R);
NagyDonat wrote:
Actually, I realized that it's better to avoid asserting that `V` is not
unknown. This is a precondition that _happens to be satisfied_ because this
class is constructed in one single location and that is preceded by a
`!V.isUnknown()` check -- but I'm fairly certain that the actual implementation
of `StoreSiteFinder` can find a site where `UnknownVal` was stored in a region.
https://github.com/llvm/llvm-project/pull/86953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
https://github.com/haoNoQ approved this pull request. Yeah nuke it. https://github.com/llvm/llvm-project/pull/86953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
@@ -1238,12 +1238,12 @@ class StoreSiteFinder final : public
TrackingBugReporterVisitor {
///changes to its value in a nested stackframe could be pruned, and
///this visitor can prevent that without polluting the bugpath too
///much.
- StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
+ StoreSiteFinder(bugreporter::TrackerRef ParentTracker, SVal V,
const MemRegion *R, TrackingOptions Options,
const StackFrameContext *OriginSFC = nullptr)
: TrackingBugReporterVisitor(ParentTracker), R(R), V(V),
Options(Options),
OriginSFC(OriginSFC) {
-assert(R);
+assert(!V.isUnknown() && R);
steakhal wrote:
Split these conditions to different assertions.
https://github.com/llvm/llvm-project/pull/86953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
https://github.com/steakhal approved this pull request. The simpler the better. Thanks. https://github.com/llvm/llvm-project/pull/86953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/86953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (NagyDonat)
Changes
The class `KnownSVal` was very magical abstract class within the `SVal` class
hierarchy: with a hacky `classof` method it acted as if it was the common
ancestor of the classes `UndefinedSVal` and `DefinedSVal`.
However, it was only used in two `getAsKnownSVal()` calls and the
signatures of two methods, which does not "pay for" its weird behavior, so I
created this commit that removes it and replaces its use with more
straightforward solutions.
---
Full diff: https://github.com/llvm/llvm-project/pull/86953.diff
4 Files Affected:
- (modified)
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
(+2-1)
- (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h (-8)
- (modified)
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
(+1-1)
- (modified) clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (+7-7)
``diff
diff --git
a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index d9b3d9352d3224..cc3d93aabafda4 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -374,6 +374,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr
*E,
/// from.
///
/// \param V We're searching for the store where \c R received this value.
+///It may be either defined or undefined, but should not be unknown.
/// \param R The region we're tracking.
/// \param Opts Tracking options specifying how we want to track the value.
/// \param Origin Only adds notes when the last store happened in a
@@ -383,7 +384,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr
*E,
///changes to its value in a nested stackframe could be pruned, and
///this visitor can prevent that without polluting the bugpath too
///much.
-void trackStoredValue(KnownSVal V, const MemRegion *R,
+void trackStoredValue(SVal V, const MemRegion *R,
PathSensitiveBugReport , TrackingOptions Opts =
{},
const StackFrameContext *Origin = nullptr);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
index c60528b7685fe8..3a4b0872571494 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -232,14 +232,6 @@ class DefinedSVal : public DefinedOrUnknownSVal {
: DefinedOrUnknownSVal(Kind, Data) {}
};
-/// Represents an SVal that is guaranteed to not be UnknownVal.
-class KnownSVal : public SVal {
-public:
- /*implicit*/ KnownSVal(DefinedSVal V) : SVal(V) {}
- /*implicit*/ KnownSVal(UndefinedVal V) : SVal(V) {}
- static bool classof(SVal V) { return !V.isUnknown(); }
-};
-
class NonLoc : public DefinedSVal {
protected:
NonLoc(SValKind Kind, const void *Data) : DefinedSVal(Kind, Data) {}
diff --git
a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
index c3acb73ba7175b..086c3e5e49b770 100644
---
a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -977,7 +977,7 @@ void RefLeakReport::findBindingToReport(CheckerContext ,
// something like derived regions if we want to construct SVal from
// Sym. Instead, we take the value that is definitely stored in that
// region, thus guaranteeing that trackStoredValue will work.
-bugreporter::trackStoredValue(AllVarBindings[0].second.castAs(),
+bugreporter::trackStoredValue(AllVarBindings[0].second,
AllocBindingToReport, *this);
} else {
AllocBindingToReport = AllocFirstBinding;
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index a0822513a6d02e..ce167d979d5761 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1238,12 +1238,12 @@ class StoreSiteFinder final : public
TrackingBugReporterVisitor {
///changes to its value in a nested stackframe could be pruned, and
///this visitor can prevent that without polluting the bugpath too
///much.
- StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
+ StoreSiteFinder(bugreporter::TrackerRef ParentTracker, SVal V,
const MemRegion *R, TrackingOptions Options,
const StackFrameContext *OriginSFC = nullptr)
: TrackingBugReporterVisitor(ParentTracker),
[clang] [analyzer] Remove barely used class 'KnownSVal' (NFC) (PR #86953)
https://github.com/NagyDonat created
https://github.com/llvm/llvm-project/pull/86953
The class `KnownSVal` was very magical abstract class within the `SVal` class
hierarchy: with a hacky `classof` method it acted as if it was the common
ancestor of the classes `UndefinedSVal` and `DefinedSVal`.
However, it was only used in two `getAs()` calls and the signatures
of two methods, which does not "pay for" its weird behavior, so I created this
commit that removes it and replaces its use with more straightforward solutions.
>From 638497ef227cf6f3e8d558618a186a3c45935dfc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Thu, 28 Mar 2024 14:49:15 +0100
Subject: [PATCH] [analyzer] Remove barely used class 'KnownSVal' (NFC)
The class `KnownSVal` was very magical abstract class within the `SVal`
class hierarchy: with a hacky `classof` method it acted as if it was the
common ancestor of the classes `UndefinedSVal` and `DefinedSVal`.
However, it was only used in two `getAs()` calls and the
signatures of two methods, which does not "pay for" its weird behavior,
so I created this commit that removes it and replaces its use with more
straightforward solutions.
---
.../Core/BugReporter/BugReporterVisitors.h | 3 ++-
.../StaticAnalyzer/Core/PathSensitive/SVals.h | 8
.../RetainCountChecker/RetainCountDiagnostics.cpp | 2 +-
.../StaticAnalyzer/Core/BugReporterVisitors.cpp| 14 +++---
4 files changed, 10 insertions(+), 17 deletions(-)
diff --git
a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index d9b3d9352d3224..cc3d93aabafda4 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -374,6 +374,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr
*E,
/// from.
///
/// \param V We're searching for the store where \c R received this value.
+///It may be either defined or undefined, but should not be unknown.
/// \param R The region we're tracking.
/// \param Opts Tracking options specifying how we want to track the value.
/// \param Origin Only adds notes when the last store happened in a
@@ -383,7 +384,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr
*E,
///changes to its value in a nested stackframe could be pruned, and
///this visitor can prevent that without polluting the bugpath too
///much.
-void trackStoredValue(KnownSVal V, const MemRegion *R,
+void trackStoredValue(SVal V, const MemRegion *R,
PathSensitiveBugReport , TrackingOptions Opts =
{},
const StackFrameContext *Origin = nullptr);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
index c60528b7685fe8..3a4b0872571494 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -232,14 +232,6 @@ class DefinedSVal : public DefinedOrUnknownSVal {
: DefinedOrUnknownSVal(Kind, Data) {}
};
-/// Represents an SVal that is guaranteed to not be UnknownVal.
-class KnownSVal : public SVal {
-public:
- /*implicit*/ KnownSVal(DefinedSVal V) : SVal(V) {}
- /*implicit*/ KnownSVal(UndefinedVal V) : SVal(V) {}
- static bool classof(SVal V) { return !V.isUnknown(); }
-};
-
class NonLoc : public DefinedSVal {
protected:
NonLoc(SValKind Kind, const void *Data) : DefinedSVal(Kind, Data) {}
diff --git
a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
index c3acb73ba7175b..086c3e5e49b770 100644
---
a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -977,7 +977,7 @@ void RefLeakReport::findBindingToReport(CheckerContext ,
// something like derived regions if we want to construct SVal from
// Sym. Instead, we take the value that is definitely stored in that
// region, thus guaranteeing that trackStoredValue will work.
-bugreporter::trackStoredValue(AllVarBindings[0].second.castAs(),
+bugreporter::trackStoredValue(AllVarBindings[0].second,
AllocBindingToReport, *this);
} else {
AllocBindingToReport = AllocFirstBinding;
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index a0822513a6d02e..ce167d979d5761 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1238,12 +1238,12 @@ class StoreSiteFinder final : public
