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

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

steakhal added a comment.

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.

The test coverage looks good to me.

Good job.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1641
+    // If both have different signs then only we can get more information.
+    if (LHS.isUnsigned() ^ RHS.isUnsigned()) {
+      if (LHS.isUnsigned() && (LHS.getBitWidth() >= RHS.getBitWidth())) {
----------------
I think in this context `!=` should achieve the same, and we usually prefer this operator for this.


================
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:
> 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.


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


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1644-1649
+        // If signed range is <Zero, then we can simply infer that expression
+        // will return true.
+        llvm::APSInt Zero = RHS.getAPSIntType().getZeroValue();
+        bool IsRHSNegative = RHS.getMaxValue() < Zero;
+        if (IsRHSNegative)
+          return getTrueRange(T);
----------------



================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140086