[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-28 Thread Gábor Spaits via cfe-commits 

https://github.com/spaits closed https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-12 Thread via cfe-commits 

DonatNagyE wrote:

I'm also weakly opposed to this patch, because the status quo is slightly 
better than introducing an assert that would make this area more fragile and 
could lead to some assertion failures if this code is modified in the future.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 

https://github.com/spaits updated 
https://github.com/llvm/llvm-project/pull/75076

From f3d7181073996dd94e6d0b7ac403e9a3d97f4e0d Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 16:59:16 +0100
Subject: [PATCH 1/3] [Analyzer][NFC] Remove redundant function call

PRValueHandler's handle function is only called from
Tracker's track function. In Tracker::track the
ExprNode parameter passed to PRValueHandler::handle
is already the ExplodedNode for the tracked expression.
We do not need to calculate that again.
---
 .../lib/StaticAnalyzer/Core/BugReporterVisitors.cpp  | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a9d130c240aec..4a1607530dbd82 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2565,21 +2565,17 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
-
-const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
-if (!RVNode)
+if (!E->isPRValue() || !RVNode)
   return {};
 
 Tracker::Result CombinedResult;
 Tracker  = getParentTracker();
 
-const auto track = [, , ExprNode,
+const auto track = [, , RVNode,
 Opts](const Expr *Inner) {
-  CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
+  CombinedResult.combineWith(Parent.track(Inner, RVNode, Opts));
 };
 
 // FIXME: Initializer lists can appear in many different contexts

From 0f36d7e1947380c39f062840ad75bf7bfd8b0bbd Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 19:14:36 +0100
Subject: [PATCH 2/3] Add assertion to check if RVNode node for E

---
 clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a1607530dbd82..df2be72531e35f 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2567,6 +2567,9 @@ class PRValueHandler final : public ExpressionHandler {
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
  const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
+assert(RVNode->getStmtForDiagnostics() == E &&
+   "RVNode must be the ExplodedNode for the tracked expression.");
+
 if (!E->isPRValue() || !RVNode)
   return {};
 

From 3bce8ce37cff49e482700684462193adb8326df9 Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 19:35:39 +0100
Subject: [PATCH 3/3] Move RVNode nullptr check

---
 clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index df2be72531e35f..4dbbd2024577e1 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2567,12 +2567,13 @@ class PRValueHandler final : public ExpressionHandler {
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
  const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-assert(RVNode->getStmtForDiagnostics() == E &&
-   "RVNode must be the ExplodedNode for the tracked expression.");
 
 if (!E->isPRValue() || !RVNode)
   return {};
 
+assert(RVNode->getStmtForDiagnostics() == E &&
+   "RVNode must be the ExplodedNode for the tracked expression.");
+
 Tracker::Result CombinedResult;
 Tracker  = getParentTracker();
 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 


@@ -2565,21 +2565,24 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
 
-const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
 if (!RVNode)
   return {};
 
+assert(RVNode->getStmtForDiagnostics() == E &&
+   "RVNode must be the ExplodedNode for the tracked expression.");
+
+if (!E->isPRValue())

isuckatcs wrote:

This should still go first. If `E` is not a `PRValue`, this handler should not 
act on it at all.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 

isuckatcs wrote:

> Consider downstream users that might use this reporting system and have their 
> own trackers. (We don't at Sonar, but pretend), then they would need to 
> remove one more unjust assert.

This is also a fair point, though as far as I know, LLVM doesn't keep API 
compatibility either between releases, so downstream users should expect that 
their code will need adjustments.

> Now we could save the cost for building a call stack, a comparison, a 
> conditional jump based on the result of the previous comparison and also the 
> deconstruction of the stack.

Actually a release build compiled with `-O2` or `-O3` should have this function 
inlined already, so the stack frame is not created. As for the others, I'm not 
sure if that will be a significant or even a measurable performance 
improvement. When it comes to speeding up execution, the rule of thumb is to 
always measure it though.

[D131707](https://reviews.llvm.org/D131707) is/was a patch, where improving 
runtime speed came up as a discussion too. It contains some points about 
performance bottlenecks in the analyzer, tips on how to get started with 
profiling, etc. If you're interested in such stuff, it might worth taking a 
look at.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 

spaits wrote:

My rationale behind this change is that, as I mentioned this class is for a 
very specific purpose. It is a module of sub-system, not a generic "library" 
class that is intended to be used from anywhere, so this class would not really 
make sense to be used anywhere else.

If we take a look at the definition of the `ExpressionHandler` base class in 
`BugReporterVisitor.h`, then we can see that the third argument should be the 
node where the evaluation of the expression happens. We should not look for 
that node in the function. It is not the function's responsibility but the 
caller's responsibility.
```C++
  /// Handle the given expression from the given node.
  ///
  /// \param E The expression value which we are tracking
  /// \param Original A node "downstream" where the tracking started.
  /// \param ExprNode A node where the evaluation of \c E actually happens.
  /// \param Opts Tracking options specifying how we are tracking the value.
  virtual Tracker::Result handle(const Expr *E, const ExplodedNode *Original,
 const ExplodedNode *ExprNode,
 TrackingOptions Opts) = 0;
```
So there is already a written precondition for that parameter. I think the best 
way to express this precondition in C++17 is an assertion. If the user of the 
function call it incorrectly they should be warned as strongly as possible. 
With the least amount of runtime cost. Now we could save the cost for building 
a call stack, a comparison, a conditional jump based on the result of the 
previous comparison and also the deconstruction of the stack.

I also respect and accept your opinions and understand why you prefer that 
extra function call.

I will wait a bit with this PR. If no one else will review it in the next few 
weeks I will just abort it.

Thank you for your time and effort.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Balazs Benics via cfe-commits 

https://github.com/steakhal commented:

I would agree with @isuckatcs, and I'd be weak against this PR.
Right now I don't see the benefit of asserting this.
Consider downstream users that might use this reporting system and have their 
own trackers. (We don't at Sonar, but pretend), then they would need to remove 
one more unjust assert.
Speaking of assert, 
[here](https://discourse.llvm.org/t/rfc-error-handling-in-release-builds-aka-can-i-use-lldbassert-or-not/74738/10)
 is my statement about them for the whole llvm prohect.
We are on the cold path anyways here. It should not matter.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 


@@ -2565,21 +2565,20 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
+assert(RVNode->getStmtForDiagnostics() == E &&

spaits wrote:

Moved nullptr check before call.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 

https://github.com/spaits updated 
https://github.com/llvm/llvm-project/pull/75076

From f3d7181073996dd94e6d0b7ac403e9a3d97f4e0d Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 16:59:16 +0100
Subject: [PATCH 1/3] [Analyzer][NFC] Remove redundant function call

PRValueHandler's handle function is only called from
Tracker's track function. In Tracker::track the
ExprNode parameter passed to PRValueHandler::handle
is already the ExplodedNode for the tracked expression.
We do not need to calculate that again.
---
 .../lib/StaticAnalyzer/Core/BugReporterVisitors.cpp  | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a9d130c240ae..4a1607530dbd8 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2565,21 +2565,17 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
-
-const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
-if (!RVNode)
+if (!E->isPRValue() || !RVNode)
   return {};
 
 Tracker::Result CombinedResult;
 Tracker  = getParentTracker();
 
-const auto track = [, , ExprNode,
+const auto track = [, , RVNode,
 Opts](const Expr *Inner) {
-  CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
+  CombinedResult.combineWith(Parent.track(Inner, RVNode, Opts));
 };
 
 // FIXME: Initializer lists can appear in many different contexts

From 0f36d7e1947380c39f062840ad75bf7bfd8b0bbd Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 19:14:36 +0100
Subject: [PATCH 2/3] Add assertion to check if RVNode node for E

---
 clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a1607530dbd8..df2be72531e35 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2567,6 +2567,9 @@ class PRValueHandler final : public ExpressionHandler {
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
  const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
+assert(RVNode->getStmtForDiagnostics() == E &&
+   "RVNode must be the ExplodedNode for the tracked expression.");
+
 if (!E->isPRValue() || !RVNode)
   return {};
 

From 10cdc26eeb9925cd11bb85f25509ad1a751c99db Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 19:35:39 +0100
Subject: [PATCH 3/3] Move RVNode nullptr check

---
 clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index df2be72531e35..fb8630884d994 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2567,10 +2567,14 @@ class PRValueHandler final : public ExpressionHandler {
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
  const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
+
+if (!RVNode)
+  return {};
+
 assert(RVNode->getStmtForDiagnostics() == E &&
"RVNode must be the ExplodedNode for the tracked expression.");
 
-if (!E->isPRValue() || !RVNode)
+if (!E->isPRValue())
   return {};
 
 Tracker::Result CombinedResult;

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 


@@ -2565,21 +2565,20 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
+assert(RVNode->getStmtForDiagnostics() == E &&

isuckatcs wrote:

Actually, this will crash if `RVNode` is a `nullptr`, which is only checked 
afterwards.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 

isuckatcs wrote:

> I have also thought about a possible different caller but I think it is very 
> unlikely. These `ExpressionHandler`s are basically visitor like classes for 
> this tracking mechanism, if I am correct. Their purpose is to track other 
> values or append `BugReporterVisitor`s to the `PathSensitiveBugReport`.

Still, here we technically want to turn a safety check into a contract between 
the function and it's caller. With an assertion added, the callers now know 
about the contract at the cost of a possible crash. However a contract is still 
a weaker guarantee than a safety check. 

I don't see why having that safety check bothers us. It's not affecting 
performance in any significant way, the code is not harder to understand 
because of it, the function is not less maintainable, etc.

For now, I would like to know what the others think about this change too.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 

https://github.com/spaits updated 
https://github.com/llvm/llvm-project/pull/75076

From f3d7181073996dd94e6d0b7ac403e9a3d97f4e0d Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 16:59:16 +0100
Subject: [PATCH 1/2] [Analyzer][NFC] Remove redundant function call

PRValueHandler's handle function is only called from
Tracker's track function. In Tracker::track the
ExprNode parameter passed to PRValueHandler::handle
is already the ExplodedNode for the tracked expression.
We do not need to calculate that again.
---
 .../lib/StaticAnalyzer/Core/BugReporterVisitors.cpp  | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a9d130c240aec..4a1607530dbd82 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2565,21 +2565,17 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
-
-const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
-if (!RVNode)
+if (!E->isPRValue() || !RVNode)
   return {};
 
 Tracker::Result CombinedResult;
 Tracker  = getParentTracker();
 
-const auto track = [, , ExprNode,
+const auto track = [, , RVNode,
 Opts](const Expr *Inner) {
-  CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
+  CombinedResult.combineWith(Parent.track(Inner, RVNode, Opts));
 };
 
 // FIXME: Initializer lists can appear in many different contexts

From 0f36d7e1947380c39f062840ad75bf7bfd8b0bbd Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 19:14:36 +0100
Subject: [PATCH 2/2] Add assertion to check if RVNode node for E

---
 clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a1607530dbd82..df2be72531e35f 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2567,6 +2567,9 @@ class PRValueHandler final : public ExpressionHandler {
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
  const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
+assert(RVNode->getStmtForDiagnostics() == E &&
+   "RVNode must be the ExplodedNode for the tracked expression.");
+
 if (!E->isPRValue() || !RVNode)
   return {};
 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff a4e67de96f0a9833756b6c79fff3cd6ee459fee0 
d3b3bbce9ee6fa107c6bfa183dd7885f191300d6 -- 
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
``





View the diff from clang-format here.


``diff
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 745a9f34f6..df2be72531 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2567,7 +2567,8 @@ public:
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
  const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-assert(RVNode->getStmtForDiagnostics() == E && "RVNode must be the 
ExplodedNode for the tracked expression.");
+assert(RVNode->getStmtForDiagnostics() == E &&
+   "RVNode must be the ExplodedNode for the tracked expression.");
 
 if (!E->isPRValue() || !RVNode)
   return {};

``




https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 

https://github.com/spaits updated 
https://github.com/llvm/llvm-project/pull/75076

From f3d7181073996dd94e6d0b7ac403e9a3d97f4e0d Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 16:59:16 +0100
Subject: [PATCH 1/2] [Analyzer][NFC] Remove redundant function call

PRValueHandler's handle function is only called from
Tracker's track function. In Tracker::track the
ExprNode parameter passed to PRValueHandler::handle
is already the ExplodedNode for the tracked expression.
We do not need to calculate that again.
---
 .../lib/StaticAnalyzer/Core/BugReporterVisitors.cpp  | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a9d130c240aec..4a1607530dbd82 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2565,21 +2565,17 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
-
-const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
-if (!RVNode)
+if (!E->isPRValue() || !RVNode)
   return {};
 
 Tracker::Result CombinedResult;
 Tracker  = getParentTracker();
 
-const auto track = [, , ExprNode,
+const auto track = [, , RVNode,
 Opts](const Expr *Inner) {
-  CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
+  CombinedResult.combineWith(Parent.track(Inner, RVNode, Opts));
 };
 
 // FIXME: Initializer lists can appear in many different contexts

From d3b3bbce9ee6fa107c6bfa183dd7885f191300d6 Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 19:14:36 +0100
Subject: [PATCH 2/2] Add assertion to check if RVNode node for E

---
 clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a1607530dbd82..745a9f34f6a61d 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2567,6 +2567,8 @@ class PRValueHandler final : public ExpressionHandler {
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
  const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
+assert(RVNode->getStmtForDiagnostics() == E && "RVNode must be the 
ExplodedNode for the tracked expression.");
+
 if (!E->isPRValue() || !RVNode)
   return {};
 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 

spaits wrote:

It is a great idea to use assertions here. It is exactly the context in which 
they should be used. I will add that. Thank you for the review.

I have also thought about a possible different caller but I think it is very 
unlikely. These `ExpressionHandler`s are basically visitor like classes for 
this tracking mechanism, if I am correct. Their purpose is to track other 
values or append `BugReporterVisitor`s to the `PathSensitiveBugReport`.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 

https://github.com/isuckatcs requested changes to this pull request.

.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 

isuckatcs wrote:

If it's guaranteed that `ExprNode` is for `E`, I think it would be a good idea 
to assert it in the function.

```c++
assert(ExprNode->getStmtForDiagnostics() == E);
```

However I guess this assumption is made from the only currently known callsite 
of the function, which is:
```c++
Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N,
   TrackingOptions Opts) {
  if (!E || !N)
return {};

  const Expr *Inner = peelOffOuterExpr(E, N); 
  const ExplodedNode *LVNode = findNodeForExpression(N, Inner); // <- Assertion 
guaranteed here
  if (!LVNode)
return {};

  Result CombinedResult;
  // Iterate through the handlers in the order according to their priorities.
  for (ExpressionHandlerPtr  : ExpressionHandlers) {
CombinedResult.combineWith(Handler->handle(Inner, N, LVNode, Opts));
if (CombinedResult.WasInterrupted) {
  // There is no need to confuse our users here.
  // We got interrupted, but our users don't need to know about it.
  CombinedResult.WasInterrupted = false;
  break;
}
  }

  return CombinedResult;
}
```
Calling the function from `Tracker::track()` indeed guarantees the assertion, 
however I can't find anything that guarantees that `PRValueHandler::handle()` 
cannot be called from anywhere else. In which case the assertion will fail and 
we'll start seeing crashes.

While the change has no effect now, it could introduce a crash later, so I 
would leave `PRValueHandler::handle()` as it is.

https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread via cfe-commits 

llvmbot wrote:




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

Author: Gábor Spaits (spaits)


Changes

`PRValueHandler`'s handle function is only called from Tracker's track 
function. In `Tracker::track` the
`ExprNode` parameter passed to `PRValueHandler::handle` is already the 
`ExplodedNode` for the tracked expression. We do not need to calculate that 
again.

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


1 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (+4-8) 


``diff
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a9d130c240aec..4a1607530dbd82 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2565,21 +2565,17 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
-
-const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
-if (!RVNode)
+if (!E->isPRValue() || !RVNode)
   return {};
 
 Tracker::Result CombinedResult;
 Tracker  = getParentTracker();
 
-const auto track = [, , ExprNode,
+const auto track = [, , RVNode,
 Opts](const Expr *Inner) {
-  CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
+  CombinedResult.combineWith(Parent.track(Inner, RVNode, Opts));
 };
 
 // FIXME: Initializer lists can appear in many different contexts

``




https://github.com/llvm/llvm-project/pull/75076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

2023-12-11 Thread Gábor Spaits via cfe-commits 

https://github.com/spaits created 
https://github.com/llvm/llvm-project/pull/75076

`PRValueHandler`'s handle function is only called from Tracker's track 
function. In `Tracker::track` the
`ExprNode` parameter passed to `PRValueHandler::handle` is already the 
`ExplodedNode` for the tracked expression. We do not need to calculate that 
again.

From f3d7181073996dd94e6d0b7ac403e9a3d97f4e0d Mon Sep 17 00:00:00 2001
From: Gabor Spaits 
Date: Mon, 11 Dec 2023 16:59:16 +0100
Subject: [PATCH] [Analyzer][NFC] Remove redundant function call

PRValueHandler's handle function is only called from
Tracker's track function. In Tracker::track the
ExprNode parameter passed to PRValueHandler::handle
is already the ExplodedNode for the tracked expression.
We do not need to calculate that again.
---
 .../lib/StaticAnalyzer/Core/BugReporterVisitors.cpp  | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 4a9d130c240aec..4a1607530dbd82 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2565,21 +2565,17 @@ class PRValueHandler final : public ExpressionHandler {
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
- const ExplodedNode *ExprNode,
+ const ExplodedNode *RVNode,
  TrackingOptions Opts) override {
-if (!E->isPRValue())
-  return {};
-
-const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
-if (!RVNode)
+if (!E->isPRValue() || !RVNode)
   return {};
 
 Tracker::Result CombinedResult;
 Tracker  = getParentTracker();
 
-const auto track = [, , ExprNode,
+const auto track = [, , RVNode,
 Opts](const Expr *Inner) {
-  CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
+  CombinedResult.combineWith(Parent.track(Inner, RVNode, Opts));
 };
 
 // FIXME: Initializer lists can appear in many different contexts

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits