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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
