manas added inline comments.
================ Comment at: clang/test/Analysis/constant-folding.c:282 + + if (c >= -20 && d >= -40) { + clang_analyzer_eval((c + d) < -1); // expected-warning{{TRUE}} ---------------- vsavchenko wrote: > manas wrote: > > vsavchenko wrote: > > > Great, it's good to check negative numbers. > > > I have one note here. The fact that we don't have a testing framework > > > and use this approach of inspecting the actual analysis has an > > > interesting implication. > > > > > > ``` > > > if (a == 10) { // here we split path into 2 paths: one where a == 10, and > > > one where a != 10; > > > // one goes inside of if, one doesn't > > > . . . > > > } > > > if (a >= 5) { // here we split into 3 paths: a == 10, a < 5, and a in [5, > > > 9] and [11, INT_MAX] (aka a >= 5 and a != 10). > > > // 1 path was infeasible: a == 10 and a < 5 > > > // Two of these paths go inside of the if, one doesn't > > > . . . > > > clang_analyzer_eval(a == 10); // it will produce two results: TRUE and > > > FALSE > > > } > > > clang_analyzer_eval(a == 10); // it will produce three results: TRUE, > > > FALSE, and FALSE > > > ``` > > > > > > We don't want to test how path splitting works in these particular tests > > > (they are about solver and constant folding after all), that's why we try > > > to have only one path going through each `clang_analyzer_eval(...)` > > > > > > In this example, you still have residual paths where `c != INT_MIN`, `c > > > == INT_MIN and d != INT_MIN`, and `c == INT_MIN and d == INT_MIN`. > > I should add tests for these paths as well so that these can be checked. > > For further cases, I will enforce single path evaluation in test cases > > (which will make it easier to handle here). > In these tests we should really avoid having multiple feasible paths going > through one `clang_analyzer_eval`. We can add `// expected-warning` to it > multiple times, but it's harder to understand as a reader of these tests > because you need to split all the paths in your mind just like the analyzer > does. It can be useful when testing other features of the analyzer, but here > it's not essential. > > And I don't think that you fully understood me. When multiple paths go > through `clang_analyzer_eval`, each of them produces a separate "warning". > That's why if you want to account for residual paths, you shouldn't add more > `if` statements, but add something like: > ``` > clang_analyzer_eval(a == 10); // expected-warning{{TRUE}} > // expected-warning@-1{{TRUE}} > // expected-warning@-2{{FALSE}} > ``` > > So, please, instead of writing tests like this, rewrite your test, so that we > don't have residual paths. Understood. I am changing those for single path evaluation accordingly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103440/new/ https://reviews.llvm.org/D103440 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits