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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

So, like, I think it's a start. You introduced a single source of truth for casting `SVal`s and it's good. But i'm worried about our API surface.



================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:539
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` less accurately.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,
----------------
NoQ wrote:
> This is one of the most important APIs of `SValBuilder`, alongside `evalBinOp()`. The newly added behavior is definitely obscure and needs explaining. So i think you should move this comment into the header, turn it into a doxygen comment, and elaborate what "less accurately" actually means here.
Like, are you sure that the new behavior makes sense at all for call sites that aren't ex-`CastRetrievedVal`? When, and why, would a regular user use this functionality?

Maybe instead of obfuscating this functionality as a null pointer we should add an explicit flag and give it an understandable name? Even if it's `bool CastRetrievedValBackdoorHackDoNotUse = false` it's still much better than "umm we kind of expect it to work if you pass null here but we don't really know how".


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:564
   switch (V.getBaseKind()) {
+  default:
+    llvm_unreachable("Unknown SVal kind");
----------------
No, don't add `default:`. All possible cases are currently handled. It breaks compiler warning about not all cases handled in the switch when new enum cases are added. Compile-time warnings are better than run-time warnings.


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

https://reviews.llvm.org/D96090