steakhal added a comment.
I haven't looked at the implementation. I only checked the tests and something
is not quite right there. See my comments inline.
BTW the line-coverage is good. I found only two branches which I want to have
tests for - see inline.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1624
+ if (LHS.isEmpty() || RHS.isEmpty())
+ return RangeFactory.getEmptySet();
+
----------------
This branch is uncovered by tests.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1633
+ APSIntType CastingType(std::max(LHS.getBitWidth(), RHS.getBitWidth()),
+ LHS.isUnsigned() || RHS.isUnsigned());
+
----------------
Although the `RHS.isUnsigned()` is covered 2 times in the
`check-clang-analysis`target, I think it would make sense to have a dedicated
test for this case in the `constant-folding.c`.
================
Comment at: clang/test/Analysis/constant-folding.c:289-301
+ if (u1 > INT_MAX && u1 <= UINT_MAX / 2 + 4 && u1 != UINT_MAX / 2 + 2 &&
+ u1 != UINT_MAX / 2 + 3 && s1 >= INT_MIN + 1 && s1 <= INT_MIN + 2) {
+ // u1: [INT_MAX+1, INT_MAX+1]U[INT_MAX+4, INT_MAX+4],
+ // s1: [INT_MIN+1, INT_MIN+2]
+ clang_analyzer_eval(u1 != s1); // expected-warning{{TRUE}}
+ }
+
----------------
These two hunks seem to be the same. We should probably keep one.
In addition to that, clang thinks the answer for `u1 != s1` depends on the
operands.
We should probably return `UNKNOWN` for such cases.
https://godbolt.org/z/aG1oY5Mr4
================
Comment at: clang/test/Analysis/constant-folding.c:315-319
+ if (s1 < 1 && s1 > -6 && s1 != -4 && s1 != -3 &&
+ u1 > UINT_MAX - 4 && u1 < UINT_MAX - 1) {
+ // s1: [-5, -5]U[-2, 0], u1: [UINT_MAX - 3, UINT_MAX - 2]
+ clang_analyzer_eval(u1 != s1); // expected-warning{{TRUE}}
+ }
----------------
Clang thinks it depends on the operands if `u1 != s1` is true.
https://godbolt.org/z/Pf3eYKnzd
================
Comment at: clang/test/Analysis/constant-folding.c:321-325
+ if (s1 < 1 && s1 > -7 && s1 != -4 && s1 != -3 &&
+ u1 > UINT_MAX - 4 && u1 < UINT_MAX - 1) {
+ // s1: [-6, -5]U[-2, 0], u1: [UINT_MAX - 3, UINT_MAX - 2]
+ clang_analyzer_eval(u1 != s1); // expected-warning{{TRUE}}
+ }
----------------
Clang thinks it depends on the operands if `u1 != s1` is true.
https://godbolt.org/z/1YYcd1EY8
================
Comment at: clang/test/Analysis/constant-folding.c:328-332
+ if (((u1 >= 1 && u1 <= 2) || (u1 >= 8 && u1 <= 9)) &&
+ u2 >= 5 && u2 <= 6) {
+ // u1: [1, 2]U[8, 9], u2: [5, 6]
+ clang_analyzer_eval(u1 != u2); // expected-warning{{TRUE}}
+ }
----------------
Clang thinks it depends on the operands if `u1 != u2` is true.
I believe, in such cases we should have returned `UNKNOWN`:
https://godbolt.org/z/fxaT7YYob
================
Comment at: clang/test/Analysis/constant-folding.c:404-407
+ if (uch > 1 && sch < 1) {
+ // uch: [2, CHAR_MAX], sch: [SCHAR_MIN, 0]
+ clang_analyzer_eval(uch != sch); // expected-warning{{UNKNOWN}}
+ }
----------------
Shouldn't it print `TRUE`?
Clang will optimize this to `return true`. https://godbolt.org/z/6c6EPYofY
We should probably conclude the same here.
================
Comment at: clang/test/Analysis/constant-folding.c:420-423
+ if (ush > 1 && ssh < 1) {
+ // ush: [2, USHRT_MAX], ssh: [SHRT_MIN, 0]
+ clang_analyzer_eval(ush != ssh); // expected-warning{{UNKNOWN}}
+ }
----------------
It should return `TRUE`. https://godbolt.org/z/44Pax16d1
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112621/new/
https://reviews.llvm.org/D112621
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
