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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
