[PATCH] D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked an inline comment as done.
sepavloff added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:2427
+    FPOptions FPFeatures = Cast->getFPFeaturesInEffect(Info.Ctx.getLangOpts());
+    RM = FPFeatures.getRoundingMode();
+  }
----------------
rjmccall wrote:
> I think the options really need to be passed in or else correctness is somewhat doomed here.
> 
> For example, the call to CompoundAssignSubobjectHandler needs to propagate this down from the operator expression.
It is guaranteed by the way AST is built, no?

As FP options may be changed only by pragmas and the pragmas can be specified only at file or block level, all sub-expression are evaluated at the same options.


================
Comment at: clang/lib/AST/ExprConstant.cpp:2678
+    // operation. If the result is exact, it does not depend on rounding mode.
+    RM = llvm::RoundingMode::NearestTiesToEven;
+  APFloat::opStatus St;
----------------
rjmccall wrote:
> Feels like you should have a helper function like `getActiveRoundingMode(Info, FPFeatures)`
It required implementation of statically polymorphic method `Expr::getFPFeaturesInEffect` but finally code looks better.


================
Comment at: clang/lib/AST/ExprConstant.cpp:2712
+  //   initializer,
+  // the evaluation probably need to be rejected.
+
----------------
rjmccall wrote:
> Is this processing something you can extract meaningfully into a common function that works with an opStatus, so that you can just write `if (!checkFloatingPointFailure(Info, E, status, fpFeatures)) return false;` in all these places?
Good idea, thank you. I used a bit different name to have positive semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87822