Charusso added a comment. Thanks for the review!
================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48 + if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) { + CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue()); + + // If a variable is reinterpreted as a type that doesn't fit into a larger + // type evenly, round it down. + // This is a signed value, since it's used in arithmetic with signed + // indices. ---------------- NoQ wrote: > Charusso wrote: > > NoQ wrote: > > > Charusso wrote: > > > > NoQ wrote: > > > > > And then remove the manual division. > > > > Hmpf. > > > > > > > > ``` > > > > Failing Tests (7): > > > > Clang :: Analysis/misc-ps-region-store.m > > > > Clang :: Analysis/mpichecker.cpp > > > > Clang :: Analysis/outofbound.c > > > > Clang :: Analysis/rdar-6541136-region.c > > > > Clang :: Analysis/return-ptr-range.cpp > > > > Clang :: Analysis/track-control-dependency-conditions.cpp > > > > Clang :: Analysis/uninit-vals.c > > > > ``` > > > > > > > > I would pick that solution because it may be a tiny-bit faster, and > > > > then later on investigate this issue when we model more about dynamic > > > > sizes. > > > Soooooo what does it tell us about the correctness of the new > > > `evalBinOp`-based solution? > > So, when I tried to inject an `APSInt` it converted to `0` so division by > > zero made that. I felt that the implicit conversion is wonky, but dividing > > by 0, ugh. > Yay, great job figuring this out! > > Also the conversion wasn't implicit; you explicitly specified > `llvm::APSInt(...)`. I agree that this constructor is evil, though. `getQuantity()` retuns a `QuantityType`, but now I see: `typedef int64_t QuantityType`, so I was fooled. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69599/new/ https://reviews.llvm.org/D69599 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits