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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to