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

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

steakhal added a comment.

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

> In D96090#2568431 <https://reviews.llvm.org/D96090#2568431>, @steakhal wrote:
>
>> Could you please rebase this?
>
> I'll update and rebase this patch soon.
>
>> If it depends on any parent revisions please document them as well.
>
> It does. You can see this in the stack. Do I need to mention this somewhere else?

No, I probably missed that. My bad.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:101-103
   // FIXME: Make these protected again once RegionStoreManager correctly
   // handles loads from different bound value types.
   virtual SVal dispatchCast(SVal val, QualType castTy) = 0;
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > I'm not sure if this FIXME is still applicable.
> > 
> > I'm also confused about having two functions doing effectively the same thing.
> > `SimpleSValBuilder::dispatchCast` is a virtual function, which just invokes a non-virtual function `SValBuilder::evalCast`.
> > 
> > Why should it be virtual in the first place?
> I want to make the patch smaller avoiding things that could be changed individually. We can remove `dispatchCast` in the next post-cleanup patch.
Seems reasonable to me.


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

https://reviews.llvm.org/D96090