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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits