[PATCH] D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D88238#2300802 <https://reviews.llvm.org/D88238#2300802>, @sepavloff wrote:

> I would propose to put a check into `checkFloatingPointResult` like:
>
>   if (St & APFloat::opStatus::opInvalidOp) {
>     if (E is __builtin_nan() and probably other similar builtins)
>       return true;
>   ...
>
> It would allow initialization with NaN but catches other operations with it.

That seems like a fragile fix - future changes to APFloat behavior might trigger the same error. Is there some reason we are not predicating the error based on the FP exception mode?

  diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
  index 3bc649b9699..323de80a17d 100644
  --- a/clang/lib/AST/ExprConstant.cpp
  +++ b/clang/lib/AST/ExprConstant.cpp
  @@ -2439,7 +2439,8 @@ static bool checkFloatingPointResult(EvalInfo &Info, const Expr *E,
       return false;
     }
   
  -  if (St & APFloat::opStatus::opInvalidOp) {
  +  if (St & APFloat::opStatus::opInvalidOp &&
  +      FPO.getFPExceptionMode() != LangOptions::FPE_Ignore) {
       // There is no usefully definable result.
       Info.FFDiag(E);
       return false;


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

https://reviews.llvm.org/D88238