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

[Manas Gupta via Phabricator via cfe-commits]

manas added a comment.

In D106102#3029633 <https://reviews.llvm.org/D106102#3029633>, @martong wrote:

> In D106102#3028584 <https://reviews.llvm.org/D106102#3028584>, @manas wrote:
>
>> The pre-merge checks fail due to the patch being unable to get applied. The troubleshooting <https://buildkite.com/llvm-project/diff-checks/builds/65987#annotation-patch_diff> suggest to update the patch via `arc diff ```git merge-base HEAD origin``` --update D106102` and it would include all local changes into that patch. Shouldn't I avoid including all local changes, as some local changes are still WIP?
>
> I guess `HEAD` should refer to the commit that represents this patch, in this case that is `ce82443c69be`. And I believe you local changes that are not committed should not affect `arc diff` at all. But, as a precaution, you could first stash your local changes before calling `arc`. Hope this helps.

Thanks. That was helpful.



================
Comment at: clang/test/Analysis/constant-folding.c:470
+
+void testEqualityRules(unsigned int a, unsigned int b, int c, int d) {
+  // Checks when ranges are not overlapping
----------------
steakhal wrote:
> steakhal wrote:
> > 
> I would prefer `u1`, `u2`, `s1`, `s2` respectively.
> This way the name would signify the signess of the variable.
Makes sense. I have amended it.


================
Comment at: clang/test/Analysis/constant-folding.c:473
+  if (a <= 10 && b >= 20) {
+    clang_analyzer_eval((a != b) != 0); // expected-warning{{TRUE}}
+  }
----------------
steakhal wrote:
> Isn't  `a != b` enough? Why do you need the `(..) != 0` part?
Sometime ago, Valeriy and I were discussing about how solver can't reason about sub-expressions containing logical operators with ranges. Currently, only `==/!=` operators can reason about them, as they are trivial. So, this case is for `!=` operator used with sub-expressions.


================
Comment at: clang/test/Analysis/constant-folding.c:476-478
+  if (c <= INT_MIN + 10 && d >= INT_MAX - 10) {
+    clang_analyzer_eval((c != d) == 0); // expected-warning{{FALSE}}
+  }
----------------
steakhal wrote:
> How is this different compared to the previous case? The difference I can see is that now we use different constants and the `==` operator in the outer expression. None of which should really change the behavior AFAICT.
I wanted to put a signed case. Although I believe, I should have replaced `==` with `!=`.


================
Comment at: clang/test/Analysis/constant-folding.c:481
+  // Checks when ranges are completely overlapping and have more than one point
+  if (a >= 20 && a <= 50 && b >= 20 && b <= 50) {
+    clang_analyzer_eval((a != b) != 0); // expected-warning{{UNKNOWN}}
----------------
steakhal wrote:
> You could have a comment that `a: [20,50]  b:[20,50]`.
> It would be easier to comprehend than the chain of conjunctions.
> Similarly, how at L464 does.
Sure. I have amended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106102