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

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

In D112621#3160082 <https://reviews.llvm.org/D112621#3160082>, @manas wrote:

> I have made few changes:
>
> 1. The failed assertions <https://reviews.llvm.org/F19816216> due to comparison between different types have been fixed by converting all the Ranges to a given type. This will allow comparisons without throwing errors.
> 2. There was also a build error due to `explicit specialization in non-namespace scope`. This was fixed by @martong previously, but that patch led to the above mentioned bug. So I used @martong 's patch here to make GCC happy.
> 3. I have added a small check for comparison between different types.
>
> https://reviews.llvm.org/D106102 differential contains the background story.

I'm not quite happy.
Next time please make sure that the tests you provide actually exercise the changed code segments.
I deleted all the old lines of the `constant-folding.c` test file and placed a `llvm_unreachable()` into `SymbolicRangeInferrer::VisitBinaryOperator<BO_NE>()` and the test did not cause a crash, which is a clear indication that the test does not test the modified code segments.
Unless I'm overly anxious I would draw false conclusions about the results of the tests and if I approve we could get crashes and complaints, and reverts.

I would kindly ask you to double-check your tests prior to submitting them to review.

---

About the technical content of this patch.

I would recommend following the same pattern as in the rest of the functions. So, `fillGaps()` then `convert()`. That way you would reduce the time-complexity of your code to `O(1)` from `O(N)`.
I would like to ask for a coverage report that shows that all of the branches of the modified code have been exercised by the `constant-folding.c` test file.

That being said, I would like to thank you for your effort in fixing this, I'm really looking forward!



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1272-1273
+  // (x != y).
+  if ((ConvertedLHS.getMaxValue() < ConvertedRHS.getMinValue()) ||
+      (ConvertedLHS.getMinValue() > ConvertedRHS.getMaxValue())) {
+    return getTrueRange(T);
----------------
This and the subsequent similar block could be implemented by a member function of a `RangeSet`.
It's highly likely that many of these `VisitBinaryOperator<Op>()` functions could benefit from reusing them in the future if we decide to handle more of them.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1279-1281
+  if ((ConvertedLHS.getMinValue() == ConvertedRHS.getMaxValue()) &&
+      (ConvertedLHS.getMaxValue() == ConvertedRHS.getMaxValue()) &&
+      (ConvertedLHS.getMinValue() == ConvertedRHS.getMinValue())) {
----------------
`ConvertedLHS.getConcreteValue() && ConvertedLHS.getConcreteValue() == ConvertedRHS.getConcreteValue()`


================
Comment at: clang/test/Analysis/constant-folding.c:326
+  if (s1 >= -1000 && s1 <= -500 && s2 <= -500 && s2 >= -750) {
+    // s1: [-1000,-500], s2: [-500,-750]
+    clang_analyzer_eval((s1 != s2) == 0); // expected-warning{{UNKNOWN}}
----------------
I would assume from `[x, y]` that `x <= y`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112621