NagyDonat wrote: > Given the track record of unintended changes, what testing methodologies did > you follow for this PR? How can we reduce the risk of applying this change?
The analyzer engine is very difficult to test, because it is full of branches that are impossible to reach (e.g. because there are no `PostStmt<QuirkyMicrosoftFeatureExpr>` checks; or none of the checkers for a certain callback split the state etc.) or almost never reached in practice (e.g. because they require a `PosteriorlyOverconstrained` state and/or a few other vanishingly rare coincidences). I think the lit tests and our "let's analyze a dozen open source projects" CI is sufficient to guarantee that the patch won't introduce _too many_ crashes. I see a few ways to extend this test coverage (e.g. we don't have any ObjC test projects), but I think that testing runs into diminishing returns not far above the current level. If we aim for logical correctness, then our only tool is reading and reviewing the code carefully (and asking chatbots to do the same), because there are many unreachable "dead" branches whose logic cannot be tested by executing the analyzer (and – a bit ironically – we also cannot use static analysis, because they juggle complex bespoke data structures). --------- I will carefully review this PR to get an independent confirmation of its logical correctness. I intend to run our CI as a final sanity check (after my review, when claude also doesn't report anything). After these steps, I think we can confidently merge this code, because it will be much more reliable than the average of the analyzer engine, which – as my various patches show – contains many logic errors. https://github.com/llvm/llvm-project/pull/218691 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
