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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits