[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:331
+                                  SourceLocation Loc);
+
   /// Emit a conversion from the specified complex type to the specified
----------------
ebevhan wrote:
> What's the plan for the other conversions (int<->fix, float<->fix)? Functions for those as well?
> 
> What about `EmitScalarConversion`? If it cannot handle conversions of fixed-point values it should probably be made to assert, since it will likely mess up.
Ideally, my plan was to have separate functions for each cast since it seems the logic for each of them is unique, other than saturation which may be abstracted to its own function and be used by the others.

I wasn't sure if I should also place the logic for these casts in `EmitScalarConversion` since `EmitScalarConversion` seemed pretty large enough and thought it was easier if we instead had separate, self contained functions for each fixed point conversion. But I'm open for hearing ideas on different ways on implementing them.

Will add the assertion.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1052
+    } else if (IsSigned && !DstFPSema.isSigned()) {
+      Value *Zero =
+          ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth, 0));
----------------
ebevhan wrote:
> `ConstantInt::getNullValue`?
I did not know about this method. Thanks!


================
Comment at: lib/Sema/Sema.cpp:537
+  case Type::STK_FixedPoint:
+    llvm_unreachable("Unknown cast from FixedPoint to boolean");
   }
----------------
ebevhan wrote:
> Will we want to support this?
I imagine we would want `_Bool` conversions since logical negation works on fixed point types and more people would probably assume by default for it to also implicitly be converted to a boolean as opposed to not.

I also don't think implementing in a later patch this will be too hard.


Repository:
  rC Clang

https://reviews.llvm.org/D50616