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

[Zoe Carver via Phabricator via cfe-commits]

zoecarver marked 2 inline comments as done.
zoecarver added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9616
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lround(Val.convertToDouble()), E);
+  }
----------------
lebedev.ri wrote:
> zoecarver wrote:
> > rsmith wrote:
> > > 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.
> > I've updated it to do just that: if there is an error, it will treat this evaluation as non-constant and leave it for the runtime library to handle. 
> > 
> > How should I determine the width of the APInt? Because it is run at compile-time, can I use the max size (long long)? Either way, how should I get the correct size, maybe from a builtin long type? 
> You probably want to look at the original callexpr, take it's return type, and look it up in typesystem (i'm not sure exactly how to do that here)
Ah, good idea. Will do.


================
Comment at: clang/test/SemaCXX/math-builtins.cpp:1
+// RUN: %clang_cc1 -std=c++11 %s
+
----------------
lebedev.ri wrote:
> -verify ?
Yes, that worked. It was throwing me off the first time I tried because it doesn't expect the nan tests to error. Any ideas on generating a nan float? Thanks for the help :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66862