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