[PATCH] D128535: [analyzer] Improve loads from reinterpret-cast fields

Mon Jul 11 04:45:26 PDT 2022

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

LGTM (given that you remove the vague and disturbing FIXME comments).

================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2014-2017
+  // FIXME: This is a hack, and doesn't do anything really intelligent yet.
+  // FIXME: This hack is analogous with the one present in
+  // `getBindingForElement`, maybe we should handle both in
+  // `getBindingForFieldOrElementCommon`.
----------------
steakhal wrote:
> martong wrote:
> > Could you please elaborate why this is a hack? What should be done and where to solve it properly? (Disclaimer, I did not look into `getBindingForFieldOrElementCommon`.)
> I'm not sure what Jordan was actually referring to. IMO hardcoding to look for a highly specific scenario (a typed memregion, and a symbol to be exact) could be thought about implementing a 'hack'. IMO this fits in the current ecosystem, so for me it doesn't look like a hack; hence IMO this is the way of implementing this.
> 
> To make it clear, I simply hoisted the nested `dyn_cast`s and checks into a function, while preserved the 'hack' comment at the callsite; so it's not added by me.
> 
> One additional note. We relay on the type information embedded into the memregion - which we know might not always be present and even accurate. That could be another reason why this should be considered as a 'hack'. IDK
Okay, thanks for the detailed explanation. So, seems like these `FIXME` comments are vague and they do not add any meaningful information, they are just disturbing noise. Could you please remove them?

Other than that, your change makes perfect sense to me.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128535