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

[Manas Gupta via Phabricator via cfe-commits]

manas marked 2 inline comments as done.
manas added a comment.

In D140086#3998426 <https://reviews.llvm.org/D140086#3998426>, @steakhal wrote:

> Thanks for going the extra mile to address this last thing. I really appreciate it.
> I've got only a few minor comments and suggestions.
>
> I'd recommend spell-checking the comments and the summary of this revision.
> See my technical comments inline.

Thank you once again for reviewing @steakhal :)

I did the spell-checking.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1641-1642
+    // If both have different signs then only we can get more information.
+    if (LHS.isUnsigned() ^ RHS.isUnsigned()) {
+      if (LHS.isUnsigned() && (LHS.getBitWidth() >= RHS.getBitWidth())) {
+
----------------
steakhal wrote:
> steakhal wrote:
> > I think in this context `!=` should achieve the same, and we usually prefer this operator for this.
> I was thinking that maybe `if (LHS.isUnsigned() && RHS.isSigned()) {} ... else if (LHS.isSigned() && RHS.isUnsigned())` results in cleaner code, as it would require one level fewer indentations.
> The control-flow looks complicated already enough.
> I was thinking that maybe if (LHS.isUnsigned() && RHS.isSigned()) {} ... else if (LHS.isSigned() && RHS.isUnsigned()) results in cleaner code

I did this, and I also combined both blocks into one to remove redundant code.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1642
+    if (LHS.isUnsigned() ^ RHS.isUnsigned()) {
+      if (LHS.isUnsigned() && (LHS.getBitWidth() >= RHS.getBitWidth())) {
+
----------------
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.




================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1666-1669
+        const llvm::APSInt CastedLHSMax =
+            RHS.getAPSIntType().convert(LHS.getMaxValue());
+        if (CastedLHSMax < RHS.getMinValue())
+          return getTrueRange(T);
----------------
steakhal wrote:
> I was thinking of using init-ifs, but on second thought I'm not sure if that's more readable.
> ```lang=c++
> if (RHS.getAPSIntType().convert(LHS.getMaxValue()) < RHS.getMinValue())
>   return getTrueRange(T);
> ```
> Shouldn't be too bad.
I think its readable. But to combine two blocks into one, I had to use different variable names, which makes this expression bigger. Should we still go with init-ifs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140086