martong added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1359 + return cache(S, + SVB.evalCast(Op, S->getType(), S->getOperand()->getType())); + } ---------------- steakhal wrote: > Hoist this common subexpression. Ok. ================ Comment at: clang/test/Analysis/svalbuilder-casts.cpp:9-10 + +// Test that the SValBuilder is able to look up and use a constraint for an +// operand of a SymbolCast, when the operand is constrained to a const value. + ---------------- steakhal wrote: > Please ass a test case where these events happen in the execution path: > 1) Have an unconstrained variable, such as a fn parameter `x`. > 2) Produce a Symbolic cast of `x`, let's call it `s`. > 3) Constrain `x` to some value range, let's say it must be positive. > 4) Verify that the value of the Symbolic cast `s` is also constrained to be > positive. Here is the test you'd like. ``` void testA(int x) { short s = x; assert(x > 0); clang_analyzer_eval(s > 0); // expected-warning{{UNKNOWN}} should be TRUE } ``` However, this will not pass, because `x` is constrained with a **range**, and in the `Simplifier` we do constant folding, i.e. the range must collapse to a single value. This test will pass with the child patch (created by Denys). ================ Comment at: clang/test/Analysis/svalbuilder-casts.cpp:44-45 + // which is not truncated to short as zero. Thus the branch is infisible. + short s = x; + if (!s) { + if (x == 65537 || x == 131073) ---------------- steakhal wrote: > It took me a while to get that the `short` variable has nothing to do with > the test. > I would recommend extending the previous example with the //short variable// > to demonstrate that the same thing (narrowing cast) can occur by either an > explicit cast or some implicit casts like a variable > initialization/assignment. Ok, I added a hunk for the implicit cast. ================ Comment at: clang/test/Analysis/svalbuilder-casts.cpp:46-49 + if (x == 65537 || x == 131073) + clang_analyzer_warnIfReached(); // no-warning + else + clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}} ---------------- steakhal wrote: > ```lang=C++ > clang_analyzer_eval(x == 1); // expected-warning {{UNKNOWN}} > clang_analyzer_eval(x == -1); // expected-warning {{UNKNOWN}} > clang_analyzer_eval(x == 0); // expected-warning {{FALSE}} > clang_analyzer_eval(x == 65537); // expected-warning {{FALSE}} > clang_analyzer_eval(x == -65537); // expected-warning {{FALSE}} > clang_analyzer_eval(x == 131073); // expected-warning {{FALSE}} > clang_analyzer_eval(x == -131073); // expected-warning {{FALSE}} > ``` Thanks, I've added these cases. > clang_analyzer_eval(x == 1); // expected-warning {{UNKNOWN}} > clang_analyzer_eval(x == -1); // expected-warning {{UNKNOWN}} Actually, `1` and `-1` does not cast to `0` as `short`, so these should be `FALSE`. Updated like so. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126481/new/ https://reviews.llvm.org/D126481 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits