rsmith added a comment.
Herald added a subscriber: dexonsmith.
The tests in this patch exhibit the same behavior with and without the patch
applied; I think almost all the functionality changes from here are superseded
by the change to consider whether we're in a manifestly constant evaluated
context. As far as I can tell, it only affects the behavior of C++ dynamic
initializers in `FENV_ACCESS ON` regions by making calls to `feset*` be
undefined behavior. I'm unconvinced that's the right way to extend the behavior
of the `FENV_*` functionality to C++. Consider this example:
#include <fenv.h>
#pragma STDC FENV_ACCESS ON
struct InRoundingMode {
int mode;
int oldmode = fegetround();
int ok1 = fesetround(mode);
double value;
int ok2 = fesetround(oldmode);
};
double d1 = InRoundingMode{.mode = FE_UPWARD, .value = 1.0 / 3.0}.value;
double d2 = InRoundingMode{.mode = FE_DOWNWARD, .value = 1.0 / 3.0}.value;
I don't think this is unreasonable: this code changes the rounding mode,
performs a calculation, and then changes it back, all within a dynamic
initializer, and all in an `FENV_ACCESS ON` region. I think it would be
unreasonable to say that the `FENV_ACCESS` doesn't apply to the initializer,
and the initializer therefore has undefined behavior.
So my inclination is to say that the status quo (prior to this patch) is
preferable behavior. The new tests look valuable.
================
Comment at: clang/test/CodeGenCXX/rounding-math.cpp:11
+
+// CHECK: @V1 = {{.*}}global float 0.000000e+00, align 4
+// CHECK: @V2 = {{.*}}global float 1.000000e+00, align 4
----------------
As discussed in other review threads, this should be a constant initializer
with value 1.0; indeed, that's what we get both with and without the code
change from this patch (this test fails when the patch is applied to Clang
trunk for this reason).
================
Comment at: clang/test/SemaCXX/rounding-math-diag.cpp:7
+ static_assert(C1 == 1.0F, ""); // expected-error{{static_assert
expression is not an integral constant expression}}
+ // expected-note@-1{{initializer of
'C1' is not a constant expression}}
+}
----------------
This is not the diagnostic we give with this patch applied (applying this patch
results in this test failing for me): we say "read of non-constexpr variable is
not allowed in a constant expression". (We don't consider the initializer of
the variable.)
================
Comment at: clang/test/SemaCXX/rounding-math.cpp:1
+// RUN: %clang_cc1 -triple x86_64-linux -verify -frounding-math
-Wno-unknown-pragmas %s
+// expected-no-diagnostics
----------------
This patch needs to be rebased; a test file with this name already exists.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88498/new/
https://reviews.llvm.org/D88498
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
