[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: clang/lib/Basic/FixedPoint.cpp:129
+      std::max(NonFractBits, OtherNonFractBits) + CommonScale;
+
+  bool ResultIsSigned = isSigned() || Other.isSigned();
----------------
ebevhan wrote:
> Does this properly compensate for types of equal width but different signedness?
> 
> If you have a signed 8-bit 7-scale fixed point number, and operate with an unsigned 8-bit integer, you'll get CommonWidth=15 and CommonScale=7, leaving 8 bits of integral. But the MSB in that cannot both be sign bit and unsigned MSB at the same time. I think you need an extra bit.
Added test for this also.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+    OtherTy = ResultTy;
+  }
+
----------------
ebevhan wrote:
> Does this handle compound assignment? The other functions handle this specially.
Currently no. I was initially intending to add addition compound assignment in a different patch, but do you think it would be appropriate to include it here? I imagine the changes to Sema wouldn't be difficult, but would probably require adding a lot more tests. 


Repository:
  rC Clang

https://reviews.llvm.org/D53738