Merge ~freddie-akeroyd/epics-base:nan_inf_tests into epics-base:7.0

Proposed by Freddie Akeroyd on 2019-09-30
Status: Rejected
Rejected by: mdavidsaver on 2020-02-14
Proposed branch: ~freddie-akeroyd/epics-base:nan_inf_tests
Merge into: epics-base:7.0
Diff against target: 127 lines (+28/-40)
2 files modified
modules/libcom/test/epicsCalcTest.cpp (+13/-14)
modules/libcom/test/epicsMathTest.c (+15/-26)
Reviewer Review Type Date Requested Status
Andrew Johnson 2019-09-30 Needs Fixing on 2019-10-04
Review via email: mp+373425@code.launchpad.net

Description of the change

Change tests to be more "a + -b" like than "a + -a" to avoid compiler optimisation issues

To post a comment you must log in.
Andrew Johnson (anj) wrote :

Group 10/4:

Suggest epicsCalcTest() should compare the string expression against the known expected result (for inf/nan calculations) so don't use Freddie's change as currently written to this file.

The epicsMathTest() changes are tricking the optimizer, which is a brittle thing. Idea is to concentrate these kinds of tests in this file, to document the known issues. As a result removing the compiler checks and calls to testToDo() from here is less helpful, we'd still like to see those until we stop building on compilers that fail. Having ToDo tests pass isn't a problem.

AI on MAD to merge and fix this.

review: Needs Fixing
mdavidsaver (mdavidsaver) wrote :

Superseded by already merged changes.

Unmerged commits

e0e6c19... by Freddie Akeroyd on 2019-09-30

Use additional local NaN/Inf in binary operations

This is to avoid issues with evaluating e.g. Inf + -Inf with
some compilers. The form "a + -b" more reflects likely
situations than "a + -a"

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/modules/libcom/test/epicsCalcTest.cpp b/modules/libcom/test/epicsCalcTest.cpp
2index 2492c95..9edc7c6 100644
3--- a/modules/libcom/test/epicsCalcTest.cpp
4+++ b/modules/libcom/test/epicsCalcTest.cpp
5@@ -294,6 +294,13 @@ static inline double MIN(double a, double b, double c, double d, double e,
6 MAIN(epicsCalcTest)
7 {
8 int repeat;
9+ /*
10+ * we need to create these to avoid issues with "NaN + -NaN" etc
11+ * and compiler optimisation. In the real world we would see a + -b
12+ * rather than a + -a
13+ * calc expressions are case insensive, so we can use "NaN + -NAN" in C
14+ */
15+ float inf = Inf, nan = NaN;
16 const double a=1.0, b=2.0, c=3.0, d=4.0, e=5.0, f=6.0,
17 g=7.0, h=8.0, i=9.0, j=10.0, k=11.0, l=12.0;
18
19@@ -612,19 +619,11 @@ MAIN(epicsCalcTest)
20 testExpr(0.0 + NaN);
21 testExpr(Inf + 0.0);
22 testExpr(Inf + Inf);
23-#if defined(_WIN64) && defined(_MSC_VER)
24- testCalc("Inf + -Inf", NaN);
25-#else
26- testExpr(Inf + -Inf);
27-#endif
28+ testExpr(Inf + -inf);
29 testExpr(Inf + NaN);
30 testExpr(-Inf + 0.0);
31-#if defined(_WIN64) && defined(_MSC_VER)
32- testCalc("-Inf + Inf", NaN);
33-#else
34- testExpr(-Inf + Inf);
35-#endif
36- testExpr(-Inf + -Inf);
37+ testExpr(-Inf + inf);
38+ testExpr(-Inf + -inf);
39 testExpr(-Inf + NaN);
40 testExpr(NaN + 0.0);
41 testExpr(NaN + Inf);
42@@ -638,11 +637,11 @@ MAIN(epicsCalcTest)
43 testExpr(0.0 - NaN);
44 testExpr(Inf - 0.0);
45 testExpr(Inf - Inf);
46- testExpr(Inf - -Inf);
47+ testExpr(Inf - -inf);
48 testExpr(Inf - NaN);
49 testExpr(-Inf - 0.0);
50- testExpr(-Inf - Inf);
51- testExpr(-Inf - -Inf);
52+ testExpr(-Inf - inf);
53+ testExpr(-Inf - -inf);
54 testExpr(-Inf - NaN);
55 testExpr(NaN - 0.0);
56 testExpr(NaN - Inf);
57diff --git a/modules/libcom/test/epicsMathTest.c b/modules/libcom/test/epicsMathTest.c
58index 8ea763c..b3e67cf 100644
59--- a/modules/libcom/test/epicsMathTest.c
60+++ b/modules/libcom/test/epicsMathTest.c
61@@ -20,6 +20,13 @@ MAIN(epicsMathTest)
62 double huge = 1e300;
63 double tiny = 1e-300;
64 double c;
65+ /*
66+ * we need to create these to avoid issues with "NaN + -NaN" etc and
67+ * compiler optimisation. In the real world we would
68+ * see a + -b rather than a + -a
69+ */
70+ float epicsINF_ = epicsINF;
71+ float epicsNAN_ = epicsNAN;
72
73 testPlan(35);
74
75@@ -28,29 +35,17 @@ MAIN(epicsMathTest)
76
77 testOk1(!isnan(epicsINF));
78 testOk1(isinf(epicsINF));
79- testOk1(epicsINF == epicsINF);
80+ testOk1(epicsINF == epicsINF_);
81 testOk1(epicsINF > 0.0);
82- testOk1(epicsINF - epicsINF != 0.0);
83+ testOk1(epicsINF - epicsINF_ != 0.0);
84
85-#if defined(_WIN64) && defined(_MSC_VER)
86- testTodoBegin("Known failure on windows-x64");
87-#endif
88- testOk1(epicsINF + -epicsINF != 0.0);
89- testOk1(-epicsINF + epicsINF != 0.0);
90-#if defined(_WIN64) && defined(_MSC_VER)
91- testTodoEnd();
92-#endif
93+ testOk1(epicsINF + -epicsINF_ != 0.0);
94+ testOk1(-epicsINF + epicsINF_ != 0.0);
95
96 testOk1(isnan(epicsINF - epicsINF));
97
98-#if defined(_WIN64) && defined(_MSC_VER)
99- testTodoBegin("Known failure on windows-x64");
100-#endif
101- testOk1(isnan(epicsINF + -epicsINF));
102- testOk1(isnan(-epicsINF + epicsINF));
103-#if defined(_WIN64) && defined(_MSC_VER)
104- testTodoEnd();
105-#endif
106+ testOk1(isnan(epicsINF + -epicsINF_));
107+ testOk1(isnan(-epicsINF + epicsINF_));
108
109 testOk1(isnan(epicsNAN));
110 testOk1(!isinf(epicsNAN));
111@@ -62,14 +57,8 @@ MAIN(epicsMathTest)
112 testOk1(!(epicsNAN > epicsNAN));
113 testOk1(isnan(epicsNAN - epicsNAN));
114
115-#if defined(_WIN64) && defined(_MSC_VER)
116- testTodoBegin("Known failure on windows-x64");
117-#endif
118- testOk1(isnan(epicsNAN + -epicsNAN));
119- testOk1(isnan(-epicsNAN + epicsNAN));
120-#if defined(_WIN64) && defined(_MSC_VER)
121- testTodoEnd();
122-#endif
123+ testOk1(isnan(epicsNAN + -epicsNAN_));
124+ testOk1(isnan(-epicsNAN + epicsNAN_));
125
126 c = huge / tiny;
127 testOk(!isnan(c), "!isnan(1e300 / 1e-300)");

Subscribers

People subscribed via source and target branches