[PATCH] D112621: [analyzer][solver] Introduce reasoning for not equal to operator

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Here are my remarks.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1415-1416
+
+  Range CoarseLHS = fillGaps(LHS);
+  Range CoarseRHS = fillGaps(RHS);
+
----------------
Avoid filling the gaps, because it's completely possible to compare all the ranges.
E.g. //LHS //`[1,2]U[8,9]`  and  //RHS //`[5,6]` are not equal.
And you don't need to fiil the gap in LHS, because it will lead you to a wrong answer, like `[1,9] != [5,6]` => **UNKNOWN** instead of **TRUE**.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1420-1421
+
+  auto ConvertedCoarseLHS = convert(CoarseLHS, ResultType);
+  auto ConvertedCoarseRHS = convert(CoarseRHS, ResultType);
+
----------------
The `ResultType` of `BO_NE` is `bool`. You don't need to convert your **integer **ranges to **boolean**. Otherwise, you'll lose the information to compare. 


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1430-1433
+  if ((ConvertedCoarseLHS->To() < ConvertedCoarseRHS->From()) ||
+      (ConvertedCoarseLHS->From() > ConvertedCoarseRHS->To())) {
+    return getTrueRange(T);
+  }
----------------
You can simply check an intersection (`RangeSet::Factory::intersect`) between the ranges.
If the result range is //empty// then return TRUE.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1437-1441
+  if (ConvertedCoarseLHS->getConcreteValue() &&
+      ConvertedCoarseLHS->getConcreteValue() ==
+          ConvertedCoarseRHS->getConcreteValue()) {
+    return getFalseRange(T);
+  }
----------------
This is OK but check `ConvertedCoarseRHS->getConcreteValue()` for `nullptr` before getting the value.



================
Comment at: clang/test/Analysis/constant-folding.c:339
+  }
+}
----------------
I'd like to see the cases with concrete integers as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112621/new/

https://reviews.llvm.org/D112621