ebevhan added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:9825
+ if (Result.isSigned() && !DstSigned) {
+ Overflow = Result < 0 || Result.ugt(DstMax);
+ } else if (Result.isUnsigned() && DstSigned) {
----------------
leonardchan wrote:
> ebevhan wrote:
> > The `Result < 0` is more clearly expressed as `Result.isNegative()` I think.
> Ah, so I ran into something similar with the patch preceding this in
> `APFixedPoint::convert()`. `isNegative()` is a method of `APInt` which
> doesn't care about signage. It just checks if the last bit is set. `Result <
> 0` calls `APSInt::operator<()` which cares about the sign and checks if this
> signed int is less than zero.
>
> have no big problem with this, but if `Result.isNegative()` is more
> readable, I could also add `Result.isSigned()` which together effectively
> does the same thing as `Result < 0`.
This makes sense, but you're already checking if the value is signed in the
line above, so it shouldn't be an issue.
================
Comment at: clang/test/Frontend/fixed_point_conversions.c:426
+ _Sat short _Accum sat_sa;
+ _Sat unsigned short _Accum sat_usa;
+
----------------
There are no tests here for what you get if you convert an integer to a
fixed-point type with a larger integral part than the integer has.
================
Comment at: clang/test/Frontend/fixed_point_conversions.c:437
+ // DEFAULT-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SATMIN]] to i16
+ // DEFAULT-NEXT: store i16 [[RES]], i16* %sat_sa, align 2
+
----------------
Conversions like this are a bit odd. There shouldn't be a need to
upsize/upscale the container before the saturation, should there?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56900/new/
https://reviews.llvm.org/D56900
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
