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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

In D88238#2300890 <https://reviews.llvm.org/D88238#2300890>, @spatel wrote:

> 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;

I have no objection to such solution, if it enables this patch.

Catching `Invalid` exception in all situations but initialization looks more natural, but this is a topic for other patches.


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

https://reviews.llvm.org/D88238