[PATCH] D140086: [analyzer][solver] Improve reasoning for not equal to operator

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

steakhal added a comment.

About spellings. In the summary you used 'lesser', I think as a synonym for 'smaller' or something like that. Anyway, not important.
Great stuff.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1642
+    if (LHS.isUnsigned() != RHS.isUnsigned()) {
+      auto SignedHS = LHS.isUnsigned() ? RHS : LHS;
+
----------------
Interesting. I like it. I'd however recommend to move this and the other variable to the beginning of this guarded block. That way it would be easier to see that the guard condition relates to this ternary condition.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1642
+    if (LHS.isUnsigned() ^ RHS.isUnsigned()) {
+      if (LHS.isUnsigned() && (LHS.getBitWidth() >= RHS.getBitWidth())) {
+
----------------
manas wrote:
> steakhal wrote:
> > Why do we need this additional condition?
> > If I remove these, I get no test failures, which suggests to me that we have some undertested code paths here.
> > Why do we need this additional condition?
> 
> Bitwidth was important because we should ideally cast smaller bitwidth type to bigger bitwidth type.
> 
> Consider if we have `LHS(u8), RHS(i32)`, then without checking for bitwidth, we would be casting RHS's maxValue to LHS's type, which will result in lose of information and will not serve our purpose.
> 
> 
If you think we need that bitwidth check, why did you remove it?
I'd like to see test cases demonstrating what we are talking about and see if we want that behavior or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140086