[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/FixedPoint.h:98
  public:
+   APFixedPoint() = default;
    APFixedPoint(const llvm::APInt &Val, const FixedPointSemantics &Sema)
----------------
This should be documented to describe what it does.  Does it create a value with impossible semantics?  Some reasonable default value in some reasonable default semantics?


================
Comment at: clang/lib/AST/ExprConstant.cpp:9953
+      return false;
+    APFixedPoint Result = Src.convert(DestFXSema);
+    return Success(Result, E);
----------------
Can a fixed-point conversion ever have undefined behavior?  If so, you might need to flag that as a failed case, unless we want to give it defined semantics in Clang.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9955
+    return Success(Result, E);
+  }
+  default:
----------------
Do you not have a separate `CK_IntegralToFixedPoint`?  It's convenient here, but still, it's curious.

You also need a `CK_FloatingToFixedPoint`, I think.  (Obviously you don't need to implement that right now.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:9959
+  }
+  llvm_unreachable("unknown cast resulting in fixed point value");
+}
----------------
This is obviously unreachable because of the `default` case in the `switch`.  Should this be moved into the `default` case, and then you can put your `Error` call in cases for the known-missing fixed-point conversions?

You should go ahead and add cases for `CK_NoOp` and `CK_LValueToRValue`, they should be obvious from the other evaluators.

You should add tests for constant-evaluating fixed-point conversions.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9982
+  }
+  llvm_unreachable("Should've exited before this");
+}
----------------
This `unreachable` seems more reasonable since you're probably never going to make this a covered `switch`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55868/new/

https://reviews.llvm.org/D55868