rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang/lib/AST/ExprConstant.cpp:2427 + FPOptions FPFeatures = Cast->getFPFeaturesInEffect(Info.Ctx.getLangOpts()); + RM = FPFeatures.getRoundingMode(); + } ---------------- sepavloff wrote: > rjmccall wrote: > > sepavloff wrote: > > > 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. > > Yes, but you can't actually reliably recover those settings from E unless > > you're sure E is one of a few kinds of expression. The concern is that E > > might end up being some closely-related expression that isn't actually the > > expression that carries the current settings, and then we'll fall back on > > using the global defaults. It's much more correct-by-construction to pass > > the settings down from the caller based on the caller's static knowledge of > > which expression is under consideration, and I think you'll see that that's > > pretty straightforward in practice. > This is a peculiarity of `CompoundAssignOperator`, which itself makes > conversion, without `CastExpr`. I added `assert` to ensure that other nodes > cannot appear here. Also some tests with conversion in > `CompoundAssignOperator` were added. Alright, well, I don't understand the reluctance to pass this down, but I won't insist. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87822/new/ https://reviews.llvm.org/D87822 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
