vabridgers marked 2 inline comments as done.
vabridgers added a comment.
Thanks Balazs, I think the comments have been addressed. Let me know if there's
anything else to do, or if this is ready to land.
Best!
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+ Loc::isLocType(T));
return APSIntType(Ctx.getIntWidth(T),
!T->isSignedIntegerOrEnumerationType());
}
----------------
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.
================
Comment at: clang/test/Analysis/fixed-point.c:6-9
+long a(int c) {
+ (long _Accum) c >> 4;
+ return c;
+}
----------------
steakhal wrote:
> It had a few extra spaces. And by casting it to void we could eliminate
> `-Wno-unused` as well.
Dang, I used git-clang-format to try cleaning the patch thinking it would clean
up formatting, but I guess it didn't :/
I cleaned up the test case and removed the -Wno-unused option.
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
