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