[PATCH] D66862: Make lround builtin constexpr (and others)

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9612
 
+  case Builtin::BIlround:
+  case Builtin::BI__builtin_lround: {
----------------
It's non-conforming to accept calls to these non-`__builtin` functions in constant expression evaluation, but it's fine to constant-fold them. Please issue a `CCEDiag` diagnostic for them (take a look at what we do for `Builtin::BIstrlen` or `Builtin::BIstrcmp` for an example; it would probably be reasonable to factor out a local lambda to issue suitable diagnostics for this case).

(For example, we are required to produce a diagnostic for `constexpr long n = lround(1.2);`, but we are permitted to constant-fold `long n = lround(1.2);` and emit it with static initialization.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:9616
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lround(Val.convertToDouble()), E);
+  }
----------------
This assumes that the host `double` has the same behavior as the target `double`, which is not true in general. Generally the right thing to do is to implement the relevant functionality on `APFloat`, taking care to produce the right answer for the floating-point model being used by that `APFloat`. This also assumes that the host `long` is the same size as the target `long`, which again is not true in general. Finally, you need to catch the case where the `APFloat` does not fit into the target type; in that case, the best response is probably to treat the evaluation as non-constant and leave it to the runtime library to implement whatever error response it chooses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66862