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());
}
----------------
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!
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
