[clang] [analyzer] Improve some comments in ArrayBoundCheckerV2 (NFC) (PR #83545)

[via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID: <llvm.org/llvm/llvm-project/pull/83545 at github.com>
In-Reply-To:


https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/83545

This comment-only change fixes a typo, clarifies some comments and includes some thoughts about the difficulties in resolving a certain FIXME.

>From b15b88d1449d81e23629d79c914e80a4ee9e19eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Fri, 1 Mar 2024 09:57:26 +0100
Subject: [PATCH 1/2] [analyzer] Fix a typo in a comment in ArrayBoundCheckerV2
 (NFC)

...which was introduced by one of my recent commits.
---
 clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
index fdcc46e58580b4..65aad35315976c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -304,7 +304,7 @@ compareValueToThreshold(ProgramStateRef State, NonLoc Value, NonLoc Threshold,
       // negative_value == unsigned_value is always false
       return {nullptr, State};
     }
-    // negative_value < unsigned_value is always false
+    // negative_value < unsigned_value is always true
     return {State, nullptr};
   }
   if (isUnsigned(SVB, Value) && isNegative(SVB, State, Threshold)) {

>From 467b9c6094018efead1981d607b67dfa174ca969 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Fri, 1 Mar 2024 10:17:20 +0100
Subject: [PATCH 2/2] [analyzer] Improve some comments in ArrayBoundCheckerV2
 (NFC)

To clarify them and include some thoughts about the difficulties in
resolving a certain FIXME.
---
 .../Checkers/ArrayBoundCheckerV2.cpp             | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
index 65aad35315976c..29eb932584027d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -301,21 +301,27 @@ compareValueToThreshold(ProgramStateRef State, NonLoc Value, NonLoc Threshold,
   // calling `evalBinOpNN`:
   if (isNegative(SVB, State, Value) && isUnsigned(SVB, Threshold)) {
     if (CheckEquality) {
-      // negative_value == unsigned_value is always false
+      // negative_value == unsigned_threshold is always false
       return {nullptr, State};
     }
-    // negative_value < unsigned_value is always true
+    // negative_value < unsigned_threshold is always true
     return {State, nullptr};
   }
   if (isUnsigned(SVB, Value) && isNegative(SVB, State, Threshold)) {
-    // unsigned_value == negative_value and unsigned_value < negative_value are
-    // both always false
+    // unsigned_value == negative_threshold and
+    // unsigned_value < negative_threshold are both always false
     return {nullptr, State};
   }
-  // FIXME: these special cases are sufficient for handling real-world
+  // FIXME: These special cases are sufficient for handling real-world
   // comparisons, but in theory there could be contrived situations where
   // automatic conversion of a symbolic value (which can be negative and can be
   // positive) leads to incorrect results.
+  // NOTE: We NEED to use the `evalBinOpNN` call in the "common" case, because
+  // we want to ensure that assumptions coming from this precondition and
+  // assumptions coming from regular C/C++ operator calls are represented by
+  // constraints on the same symbolic expression. A solution that would
+  // evaluate these "mathematical" compariosns through a separate pathway would
+  // be a step backwards in this sense.
 
   const BinaryOperatorKind OpKind = CheckEquality ? BO_EQ : BO_LT;
   auto BelowThreshold =