[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96090#2607387 <https://reviews.llvm.org/D96090#2607387>, @ASDenysPetrov wrote:

> Actually many cases don't need to know an exact **original** type. Some cases can extract the //type// from `SVal` (`SymbolVal`, `ConcreteInt`) Other cases need it from outside (`MemRegionVal`, `LocAsInteger`).
> `dispatchCast`, `evalCastFromNonLoc`, `evalCastFromLoc` have a lot of similar code which is already in `evalCast`. They also calls inside `evalCast` function. I decided to move their functionality into a new splitted version of `evalCast` to decrease complexity of comprehension and try to substitute them in the future with the single `evalCast`. But substitusion needs to deal something with absent **original** type parameter. Then I decided to add support for`evalCast` to work in both modes. Practically, new `evaCast` has additional checks and casts when the **original** type is not null.

Seems reasonable to me.

> Now `evalCastFromNonLoc` and `evalCastFromLoc` are used in `SimpleSValBuilder` functions only. I think it would be better to add an explaination to the doc like `OriginalTy.isNull() is a CastRetrievedVal backdoor hack. Do not use it.` instead of new flags. I'll make a better description, but at first I'd try to investigate how to pass an **original** type to `evalCastFromNonLoc` and `evalCastFromLoc` in `SimpleSValBuilder` functions. Or maybe some other solution.

What about not defaulting that parameter? That would better represent our expectation that it **requires** an original-type.
You could still pass a default constructed QualType at each callsite.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96090/new/

https://reviews.llvm.org/D96090