[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+           Loc::isLocType(T));
     return APSIntType(Ctx.getIntWidth(T),
                       !T->isSignedIntegerOrEnumerationType());
   }
----------------
vabridgers wrote:
> steakhal wrote:
> > I don't think you are supposed to call `isSignedIntegerOrEnumerationType()` if you have a //fixed-point// type.
> > ```lang=C++
> > inline bool Type::isSignedFixedPointType() const {
> >   if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) {
> >     return ((BT->getKind() >= BuiltinType::ShortAccum &&
> >              BT->getKind() <= BuiltinType::LongAccum) ||
> >             (BT->getKind() >= BuiltinType::ShortFract &&
> >              BT->getKind() <= BuiltinType::LongFract) ||
> >             (BT->getKind() >= BuiltinType::SatShortAccum &&
> >              BT->getKind() <= BuiltinType::SatLongAccum) ||
> >             (BT->getKind() >= BuiltinType::SatShortFract &&
> >              BT->getKind() <= BuiltinType::SatLongFract));
> >   }
> >   return false;
> > }
> > ```
> > By looking at the implementation of this, I don't think you could substitute that with `isSignedIntegerOrEnumerationType()`.
> > Am I wrong about this?
> > 
> > Please demonstrate this by tests.
> I tried using isSignedIntegerOrEnumerationType() instead of (T->isIntegralOrEnumerationType() || T->isFixedPointType() ... ), but got the same assert :/  
> 
> I corrected the formatting and expanded the test cases. 
Is hould have clarified, sorry.

My point is that for constructing the APSIntType, we calculate the bitwidth and the signedness.

My problem is that the calculation is wrong for the signedness in case we have a signed fixedpointtype.
It is wrong because we reach `isSignedIntegerOrEnumerationType()` with a fixedpoint type. For that even though its signed, it will return false!

And in the end we will have an APSIntType with the wrong signednss.

So my point is that we should probably handle fixedpoint types separately to have a distict return statement for it.
But im jumping to the solution, what I originally wanted to highlight was this.
That was why I requested changes.
And this is what I wanted to see some how intests, but I wont insist if its too difficult to craft.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759