steakhal added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1897 + QualType Ty = SubReg->getValueType(); + if (BaseTy->isScalarType() && Ty->isScalarType()) { + if (Ctx.getTypeSizeInChars(BaseTy) >= Ctx.getTypeSizeInChars(Ty)) { ---------------- BTW I'm not sure why this check is here. I would expect this to work eve without this. OOh, I think I know why. `getTypeSizeInChars` can only be called on //scalars//? ================ 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`. ---------------- 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 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