[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

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