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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D96090#2568431 <https://reviews.llvm.org/D96090#2568431>, @steakhal wrote:

> This patch preserves all previous reports as expected.

That's great results!

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



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


================
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()) {
----------------
steakhal wrote:
> You are resolving exactly this FIXME in this patch if I'm correct.
> Shouldn't you update this comment?
I am not actually sure that the function I've made is good enough and is the single one. =)


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:868-870
+      if (OriginalTy.isNull())
+        // Pass to MemRegion function.
+        return evalCastSubKind(loc::MemRegionVal(R), CastTy, OriginalTy);
----------------
steakhal wrote:
> Eh, I don't like comments preceding a single-statement block.
> It might be a personal preference though.
I am trying to highlight the redirection to another cast function. If you think it's redundant, I'll remove it.


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

https://reviews.llvm.org/D96090