manas added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1642
+ if (LHS.isUnsigned() ^ RHS.isUnsigned()) {
+ if (LHS.isUnsigned() && (LHS.getBitWidth() >= RHS.getBitWidth())) {
+
----------------
steakhal wrote:
> 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.
This test fails.
```
void testfoo(unsigned char u, signed int s) {
if (u >= 253 && u <= 255 && s < INT_MAX - 2) {
// u: [253, 254], s: [INT_MIN, INT_MAX - 2]
clang_analyzer_eval(u != s); // expected-warning{{UNKNOWN}}
// but returns TRUE
}
}
```
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
