vabridgers marked an inline comment as done.
vabridgers 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:
> > 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.
> In retrospect, your original comment was clear. I did not fully comprehend
> the comment.
>
> I'll explore a few options. I checked the unittests for a "reference", and it
> seems our static analysis unittest coverage is sparse. Not sure I need to go
> that far, but I'll explore the possibility of crafting a unittest for this
> case that demonstrates the problem and solution.
>
> Thanks again - best!
Thanks for your insightful comments, in retrospect I should have dug into this
more from the very beginning. Turns out there is an API get the fixed point
sign I should be using, and creating a simple unittest had value in working
this out (imagine that ! :) ).
So, I updated with the correction and a simple unittest.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139759/new/
https://reviews.llvm.org/D139759
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
