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

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

steakhal added a comment.

This patch preserves all previous reports as expected.
You can check it by yourself at https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=D96090&items-per-page=50.

However, I still have some minor concerns inline.



================
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;
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:727-728
+      // pointers as well.
+      // FIXME: We really need a single good function to perform casts for us
+      // correctly every time we need it.
+      if (CastTy->isPointerType() && !CastTy->isVoidPointerType()) {
----------------
You are resolving exactly this FIXME in this patch if I'm correct.
Shouldn't you update this comment?


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:868-870
+      if (OriginalTy.isNull())
+        // Pass to MemRegion function.
+        return evalCastSubKind(loc::MemRegionVal(R), CastTy, OriginalTy);
----------------
Eh, I don't like comments preceding a single-statement block.
It might be a personal preference though.


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

https://reviews.llvm.org/D96090