zoecarver marked 3 inline comments as done.
zoecarver added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:9612
+ case Builtin::BIlround:
+ case Builtin::BI__builtin_lround: {
----------------
rsmith wrote:
> 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.)
Thank you for pointing this out. I will do that in a separate patch.
================
Comment at: clang/lib/AST/ExprConstant.cpp:9616
+ return EvaluateFloat(E->getArg(0), Val, Info) &&
+ Success(lround(Val.convertToDouble()), E);
+ }
----------------
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?
================
Comment at: clang/test/SemaCXX/math-builtins.cpp:20
+
+ static_assert(__builtin_llround(float()) == 0, ""); // expected-error
{{static_assert expression is not an integral constant expression}}
+ static_assert(__builtin_llroundf(float()) == 0, ""); // expected-error
{{static_assert expression is not an integral constant expression}}
----------------
For some reasons, these `expected-error`s aren't being caught by lit. Any
ideas? Do I need to put it in a `.fail.cpp` file or add something to the `RUN:`
comment?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66862/new/
https://reviews.llvm.org/D66862
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
