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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks amazing, thanks a lot.

I have one final question: can we already remove `dispatchCast()`&Co.? I don't see any other call sites anymore. Looks like it becomes dead code after your patch.

Which is fairly shocking and mind blowing that we've organically developed two independent implementations of casting, one for RegionStore and one for everything else. I'm super happy that we're cleaning this up.



================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:564-565
   }
-
-  llvm_unreachable("Unknown SVal kind");
 }
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > You probably don't want to remove this. The same applies to similar ones.
> Yep. I'd keep it here, beacuse of compiler warnings of no-return function.
> compiler warnings of no-return function.

Aha ok, in this case it's much better to keep `llvm_unreachable` outside the switch, like here and unlike the one in `evalCastKind` functions. This way we're getting compile-time warnings for unhandled enum cases.


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

https://reviews.llvm.org/D96090