NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Great, thanks!
================
Comment at: lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp:51
+ state = state->assume(ArgSVal.castAs<DefinedOrUnknownSVal>(), true);
+ // FIXME: do we want to warn here?
+ if (!state)
----------------
That's a good question, because `assume(false)` seems much more broken than
`assert(false)`.
But because we are overaggressive on branching as compared to only relying on
presumption of code liveness (eg. splitting `void foo(x, y) { if (x) ...; if
(y) ...; }` into 4 paths is unsafe - all code may be live without all paths
being feasible - but we do that anyway, same with loops and eager assumptions
and our aliasing approach and the whole inlining thing), i think we'd rather
not warn.
I mean, due to the above, it is much more likely that the assumption is valid
and the branch on which it fails is infeasible due to some function contract we
dont' see, than that the assumption is actually violated. We may do well about
null dereferences and such, but generally the static analyzer should try to
avoid parts of code that have been carefully thought out :) We may probably
want to catch cases when the assumption fails on all paths reaching it, but we
shouldn't use symbolic execution for all-paths warnings.
https://reviews.llvm.org/D33092
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
