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