[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Leonard Chan via Phabricator via cfe-commits]

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