NoQ added a comment. Aha, great, sounds like all casting can be made more correct, not just casting on store retrieve! Maybe it's worth celebrating through extra unittests on `evalCast()`. Thank you for looking into this.
The new code should obviously be restricted into `evalCastFromLoc()` because if it's a region it's a `Loc`. You didn't need to move `castRegion()`: `StoreManager` is available in `SValBuilder` through `this->StateMgr`. I don't have a strong preference on where `castRegion()` should live; the original idea was that `StoreManager` should be allowed to have an opinion on this matter because different store managers would handle that differently; however, as of now there's only one store manager and there doesn't seem to be an interest in introducing more of them whereas the abstraction has already leaked all over the place anyway. I still have questions about the extra if-statements that you've added. Shouldn't we do `castRegion()` unconditionally, given that we're trying to cast a region and that function presumably does exactly that? I just want to avoid these situations: F13337846: photo_2020-10-16_14-19-19.jpg <https://reviews.llvm.org/F13337846> In D89055#2320363 <https://reviews.llvm.org/D89055#2320363>, @NoQ wrote: > `dispatchCast()` was supposed to be the ultimate method to do so Ugh, sorry, no, that's `evalCast()`. Like `evalBinOp()` etc. My bad. Can we also use `evalCast()`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89055/new/ https://reviews.llvm.org/D89055 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
