[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Sorry for the late response, I've been away.

LGTM, although my browser doesn't want to let me set Accepted on this.



================
Comment at: unittests/Basic/FixedPointTest.cpp:380
+}
+
+// Check the value in a given fixed point sema overflows to the saturated max
----------------
leonardchan wrote:
> ebevhan wrote:
> > Technically, neither CheckSaturatedConversion function checks whether or not the result was correct, only that it saturated.
> Could you elaborate on this? It should be correct that the value saturates to the respective min/max for the destination type right?
It should... I'm not sure what I was thinking of when I wrote this comment, and I can't think of a counterexample so it's fine as it is.


================
Comment at: unittests/Basic/FixedPointTest.cpp:506
+TEST(FixedPoint, AccConversionOverflow) {
+  // To SAcc max limit (65536)
+  CheckSaturatedConversionMax(getLAccSema(), Saturated(getAccSema()),
----------------
leonardchan wrote:
> ebevhan wrote:
> > Acc?
> Short for Accum. Changed to `Accum` since it wasn't clear at first.
Seems like you got a couple `Accumum` from that.


Repository:
  rC Clang

https://reviews.llvm.org/D48661