leonardchan 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) {
----------------
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`.
================
Comment at: clang/lib/AST/ExprConstant.cpp:9839
+ return Success(Result, E);
+ }
+
----------------
rjmccall wrote:
> Can most of this reasonably be a method on `APFixedPoint`? (And likewise for
> `CK_IntegralToFixedPoint`.) If nothing else, I suspect you are going to want
> these when it comes to things like the compound assignment operators, which
> do implicit conversions internally that aren't directly expressed in the AST.
Compressed them into `APFixedPoint::convertToInt()` and
`APFixedPoint::getFromIntValue()`
================
Comment at: clang/lib/Sema/SemaChecking.cpp:11096
+ }
+ }
}
----------------
ebevhan wrote:
> Seems like a lot of logic in these blocks is duplicated from the code in
> ExprConstant.
Yeah. I moved into `APFixedPoint::getFromIntValue()` to simplify this.
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
