NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land.
Great, thanks! I still have concerns about these functions - `isNull`, `isNonNull` etc. They do indeed provide a quick test for nullness that doesn't involve constructing a new program state. This test, however, will never be as //precise// as constructing a new state. It used to be when `RangeConstraintManager` boiled down to simple range operations but now that it grew much larger (eg., the newly added support for symbolic == and != that involves tracking equivalence classes), there's really no way to tell whether a state is going to be feasible without running the whole machine and observing its emergent behavior, which is what `assume()` does. On the other hand i still don't see any indication that `assume()` is noticeably expensive. So i'm really worried that this is a premature optimization that sacrifices correctness for nothing. So i'm in favor of phasing out these functions entirely and converting all code to always use `assume()`. This may occasionally involve untangling unwanted recursions but i hope that all recursive-ish operations can be isolated within the constraint manager itself (and possibly in checker's `evalAssume()` which we can hopefully guard against with runtime assertions). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97874/new/ https://reviews.llvm.org/D97874 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
