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
