[PATCHv3,2/3] Refactor tst-strtod-round.c for type-generic-ness

Message ID 8e5e98006235dcad1d036d3e71e51615cfd79286.1464041464.git.murphyp@linux.vnet.ibm.com
State Superseded
Headers

Commit Message

Paul E. Murphy May 23, 2016, 10:18 p.m. UTC
  Reduce much of the redundancy in this file, and attempt
to coral the type specific stuff into tst-strtod.h to ease
adding an new type.

	* stdlib/tst-strtod-round.c: Effectively rewrite
	to be type-generic.  Move, update, and simplify
	many of the macros into ...
	* stdlib/tst-strtod.h (TEST): New macro
	(LDC): Likewise.
	(RND_OKf): Likewise.
	(RND_OKd): Likewise.
	(RND_OKld): Likewise.
	(_RND_OKld): Likewise.
	(IS_RND_OK): Likewise.
	(RND_OK_DFLT): Likewise.
	(STRUCT_FOREACH_TYPE): Likewise.
	(test_exactness): New test helper structure for
	the generated tests from gen-tst-strtod-round.
	(test_results): Likewise.
	(test): Likewise.
---
 stdlib/tst-strtod-round.c | 227 +++++++++++-----------------------------------
 stdlib/tst-strtod.h       |  89 ++++++++++++++++++
 2 files changed, 144 insertions(+), 172 deletions(-)
  

Comments

Joseph Myers May 24, 2016, 3:25 p.m. UTC | #1
On Mon, 23 May 2016, Paul E. Murphy wrote:

> +/* Define the long double choose (LDC) macro
> +   to select the appropriate generated long double
> +   value from the generated test data.  */
> +#if LDBL_MANT_DIG == 53 && LDBL_MAX_EXP == 1024
> +/* This is for the long double == double format.  */
> +# define LDC(a, ...) a

> +/* This macro is used in conjunction with the output from the
> +   gen-tst-strtod-round utility to select the appropriately
> +   rounded long double value for a given format.  */
> +#define TEST(s,							\

> +struct test_exactness
> +  {
> +  STRUCT_FOREACH_TYPE ()
> +  };
> +
> +struct test_results
> +  {
> +  STRUCT_FOREACH_TYPE ()
> +  };
> +
> +struct test {
> +  const char *s;
> +  struct test_exactness exact;
> +  struct test_results r[4];
> +};

All this code seems extremely specific to the tst-strtod-round test.  I 
think it belongs there, not in a header that's supposed to be about common 
code for making multiple strtod tests type-generic.

I think the only code this patch adds to tst-strtod.h that clearly belongs 
there is the definition of STRUCT_FOREACH_TYPE.  Even the rounding mode 
handling seems very specific to tst-strtod-round (to the extent of having 
a comment about "This test").

Also, struct test_exactness previously had all its elements bool.  By 
making it use STRUCT_FOREACH_TYPE you're giving floating-point types to 
what are logically booleans, which I don't think is appropriate.
  
Paul E. Murphy May 24, 2016, 4:54 p.m. UTC | #2
On 05/24/2016 10:25 AM, Joseph Myers wrote:
> On Mon, 23 May 2016, Paul E. Murphy wrote:
> 
>> +/* Define the long double choose (LDC) macro
>> +   to select the appropriate generated long double
>> +   value from the generated test data.  */
>> +#if LDBL_MANT_DIG == 53 && LDBL_MAX_EXP == 1024
>> +/* This is for the long double == double format.  */
>> +# define LDC(a, ...) a
> 
>> +/* This macro is used in conjunction with the output from the
>> +   gen-tst-strtod-round utility to select the appropriately
>> +   rounded long double value for a given format.  */
>> +#define TEST(s,							\
> 
>> +struct test_exactness
>> +  {
>> +  STRUCT_FOREACH_TYPE ()
>> +  };
>> +
>> +struct test_results
>> +  {
>> +  STRUCT_FOREACH_TYPE ()
>> +  };
>> +
>> +struct test {
>> +  const char *s;
>> +  struct test_exactness exact;
>> +  struct test_results r[4];
>> +};
> 
> All this code seems extremely specific to the tst-strtod-round test.  I 
> think it belongs there, not in a header that's supposed to be about common 
> code for making multiple strtod tests type-generic.

My intent for the header is to move all the type specific bits into
one file to make the test themselves type-generic.  If we want it to be
exclusively common code supporting type generic tests, I should push
all of the changes to tst-strtod.h back into tst-strtod-round.c.

My thinking was that in the future, gen-tst-strtod-round could be
extended slightly to generate full test cases for strtod, and thus
replace many of the existing tests in their entirety.  Though, such
tests would probably just be added to tst-strtod-round anyways.

> 
> I think the only code this patch adds to tst-strtod.h that clearly belongs 
> there is the definition of STRUCT_FOREACH_TYPE.  Even the rounding mode 
> handling seems very specific to tst-strtod-round (to the extent of having 
> a comment about "This test").
> 
> Also, struct test_exactness previously had all its elements bool.  By 
> making it use STRUCT_FOREACH_TYPE you're giving floating-point types to 
> what are logically booleans, which I don't think is appropriate.
  
Joseph Myers May 24, 2016, 5:40 p.m. UTC | #3
On Tue, 24 May 2016, Paul E. Murphy wrote:

> My intent for the header is to move all the type specific bits into
> one file to make the test themselves type-generic.  If we want it to be
> exclusively common code supporting type generic tests, I should push
> all of the changes to tst-strtod.h back into tst-strtod-round.c.

I think it should just be common code.  The right way to make a test 
type-generic varies case by case; a type-generic test may still contain 
code relating to individual types as long as it's well-factored to make 
adding new types as simple as possible and the per-type code can't readily 
share things with other tests through a common header.

(I am expecting existing type-generic tests such as 
tst-strtod-nan-locale-main.c to be included in the addition of float128 
support, whether or not they are first adapted to use tst-strtod.h to 
avoid references to particular types within the tests.)

> My thinking was that in the future, gen-tst-strtod-round could be
> extended slightly to generate full test cases for strtod, and thus

I don't think that's appropriate.  It could plausibly test for exceptions 
/ errno in addition to what it already tests, but tests for e.g. how much 
of the string is consumed (for strings with extra text after a number), 
locales, infinities / NaNs etc. seem outside its scope.
  

Patch

diff --git a/stdlib/tst-strtod-round.c b/stdlib/tst-strtod-round.c
index a2cd2ab..101e37d 100644
--- a/stdlib/tst-strtod-round.c
+++ b/stdlib/tst-strtod-round.c
@@ -21,7 +21,6 @@ 
    declared in the headers.  */
 #define _LIBC_TEST 1
 #include <fenv.h>
-#include <float.h>
 #include <math.h>
 #include <stdbool.h>
 #include <stdio.h>
@@ -29,152 +28,61 @@ 
 #include <string.h>
 #include <math-tests.h>
 
-struct exactness
-{
-  bool f;
-  bool d;
-  bool ld;
-};
-
-struct test_results {
-  float f;
-  double d;
-  long double ld;
-};
-
-struct test {
-  const char *s;
-  struct exactness exact;
-  struct test_results rd, rn, rz, ru;
-};
-
-#if LDBL_MANT_DIG == 53 && LDBL_MAX_EXP == 1024
-# define TEST(s, fexact, fd, fn, fz, fu, dexact, dd, dn, dz, du,	\
-	      ld53exact, ld53d, ld53n, ld53z, ld53u,			\
-	      ld64iexact, ld64id, ld64in, ld64iz, ld64iu,		\
-	      ld64mexact, ld64md, ld64mn, ld64mz, ld64mu,		\
-	      ld106exact, ld106d, ld106n, ld106z, ld106u,		\
-	      ld113exact, ld113d, ld113n, ld113z, ld113u)		\
-  {									\
-    s,									\
-    { fexact, dexact, ld53exact },					\
-    { fd, dd, ld53d },							\
-    { fn, dn, ld53n },							\
-    { fz, dz, ld53z },							\
-    { fu, du, ld53u }							\
-  }
-#elif LDBL_MANT_DIG == 64 && LDBL_MAX_EXP == 16384 && LDBL_MIN_EXP == -16381
-/* This is for the Intel extended float format.  */
-# define TEST(s, fexact, fd, fn, fz, fu, dexact, dd, dn, dz, du,	\
-	      ld53exact, ld53d, ld53n, ld53z, ld53u,			\
-	      ld64iexact, ld64id, ld64in, ld64iz, ld64iu,		\
-	      ld64mexact, ld64md, ld64mn, ld64mz, ld64mu,		\
-	      ld106exact, ld106d, ld106n, ld106z, ld106u,		\
-	      ld113exact, ld113d, ld113n, ld113z, ld113u)		\
-  {									\
-    s,									\
-    { fexact, dexact, ld64iexact },					\
-    { fd, dd, ld64id },							\
-    { fn, dn, ld64in },							\
-    { fz, dz, ld64iz },							\
-    { fu, du, ld64iu }							\
-  }
-#elif LDBL_MANT_DIG == 64 && LDBL_MAX_EXP == 16384 && LDBL_MIN_EXP == -16382
-/* This is for the Motorola extended float format.  */
-# define TEST(s, fexact, fd, fn, fz, fu, dexact, dd, dn, dz, du,	\
-	      ld53exact, ld53d, ld53n, ld53z, ld53u,			\
-	      ld64iexact, ld64id, ld64in, ld64iz, ld64iu,		\
-	      ld64mexact, ld64md, ld64mn, ld64mz, ld64mu,		\
-	      ld106exact, ld106d, ld106n, ld106z, ld106u,		\
-	      ld113exact, ld113d, ld113n, ld113z, ld113u)		\
-  {									\
-    s,									\
-    { fexact, dexact, ld64mexact },					\
-    { fd, dd, ld64md },							\
-    { fn, dn, ld64mn },							\
-    { fz, dz, ld64mz },							\
-    { fu, du, ld64mu }							\
-  }
-#elif LDBL_MANT_DIG == 106 && LDBL_MAX_EXP == 1024
-# define TEST(s, fexact, fd, fn, fz, fu, dexact, dd, dn, dz, du,	\
-	      ld53exact, ld53d, ld53n, ld53z, ld53u,			\
-	      ld64iexact, ld64id, ld64in, ld64iz, ld64iu,		\
-	      ld64mexact, ld64md, ld64mn, ld64mz, ld64mu,		\
-	      ld106exact, ld106d, ld106n, ld106z, ld106u,		\
-	      ld113exact, ld113d, ld113n, ld113z, ld113u)		\
-  {									\
-    s,									\
-    { fexact, dexact, ld106exact },					\
-    { fd, dd, ld106d },							\
-    { fn, dn, ld106n },							\
-    { fz, dz, ld106z },							\
-    { fu, du, ld106u }							\
-  }
-#elif LDBL_MANT_DIG == 113 && LDBL_MAX_EXP == 16384
-# define TEST(s, fexact, fd, fn, fz, fu, dexact, dd, dn, dz, du,	\
-	      ld53exact, ld53d, ld53n, ld53z, ld53u,			\
-	      ld64iexact, ld64id, ld64in, ld64iz, ld64iu,		\
-	      ld64mexact, ld64md, ld64mn, ld64mz, ld64mu,		\
-	      ld106exact, ld106d, ld106n, ld106z, ld106u,		\
-	      ld113exact, ld113d, ld113n, ld113z, ld113u)		\
-  {									\
-    s,									\
-    { fexact, dexact, ld113exact },					\
-    { fd, dd, ld113d },							\
-    { fn, dn, ld113n },							\
-    { fz, dz, ld113z },							\
-    { fu, du, ld113u }							\
-  }
-#else
-# error "unknown long double format"
-#endif
+#include "tst-strtod.h"
 
 /* Include the generated test data.  */
 #include "tst-strtod-round-data.h"
 
+#define GEN_ONE_TEST(FSUF, FTYPE, FTOSTR, FTOSTRM, LSUF, CSUF)	\
+{								\
+  FTYPE f = strto ## FSUF (s, NULL);				\
+  if (f != expected->FSUF					\
+      || (copysign ## CSUF) (1.0 ## LSUF, f)			\
+	 != (copysign ## CSUF) (1.0 ## LSUF, expected->FSUF))	\
+    {								\
+      char efstr[FSTRLENMAX];					\
+      char fstr[FSTRLENMAX];					\
+      FTOSTR (efstr, FSTRLENMAX, "%" FTOSTRM "a", expected->FSUF); \
+      FTOSTR (fstr, FSTRLENMAX, "%" FTOSTRM "a", f);		\
+      printf ("strto" #FSUF "(%s) returned %s not %s"		\
+	      "(%s)\n", s, fstr, efstr, mode_name);		\
+      if ((round_ok & RND_OK ## FSUF) != 0 || exact->FSUF)	\
+	result = 1;						\
+      else							\
+	printf ("ignoring this inexact result\n");		\
+    }								\
+}
+
 static int
 test_in_one_mode (const char *s, const struct test_results *expected,
-		  const struct exactness *exact, const char *mode_name,
-		  bool float_round_ok, bool double_round_ok,
-		  bool long_double_round_ok)
+		  const struct test_exactness *exact, const char *mode_name,
+		  int round_ok)
 {
   int result = 0;
-  float f = strtof (s, NULL);
-  double d = strtod (s, NULL);
-  long double ld = strtold (s, NULL);
-  if (f != expected->f
-      || copysignf (1.0f, f) != copysignf (1.0f, expected->f))
-    {
-      printf ("strtof (%s) returned %a not %a (%s)\n", s, f,
-	      expected->f, mode_name);
-      if (float_round_ok || exact->f)
-	result = 1;
-      else
-	printf ("ignoring this inexact result\n");
-    }
-  if (d != expected->d
-      || copysign (1.0, d) != copysign (1.0, expected->d))
-    {
-      printf ("strtod (%s) returned %a not %a (%s)\n", s, d,
-	      expected->d, mode_name);
-      if (double_round_ok || exact->d)
-	result = 1;
-      else
-	printf ("ignoring this inexact result\n");
-    }
-  if (ld != expected->ld
-      || copysignl (1.0L, ld) != copysignl (1.0L, expected->ld))
-    {
-      printf ("strtold (%s) returned %La not %La (%s)\n", s, ld,
-	      expected->ld, mode_name);
-      if ((long_double_round_ok && LDBL_MANT_DIG != 106) || exact->ld)
-	result = 1;
-      else
-	printf ("ignoring this inexact result\n");
-    }
+  GEN_TEST_STRTOD_FOREACH (GEN_ONE_TEST);
   return result;
 }
 
+static const struct fetestmodes
+  {
+  const char *mode_name;
+  int rnd_ok;
+  int rnd_mode;
+  int rnd_i; /* Corresponding index into r array of struct test.  */
+  } modes[] = {
+    { "default rounding mode", RND_OK_DFLT, -1, 0 },
+#ifdef FE_DOWNWARD
+    { "FE_DOWNWARD", IS_RND_OK (FE_DOWNWARD), FE_DOWNWARD, 1 },
+#endif
+#ifdef FE_TOWARDZERO
+    { "FE_TOWARDZERO", IS_RND_OK (FE_TOWARDZERO), FE_TOWARDZERO, 2 },
+#endif
+#ifdef FE_UPWARD
+    { "FE_UPWARD", IS_RND_OK (FE_UPWARD), FE_UPWARD, 3 },
+#endif
+    {}
+};
+
 static int
 do_test (void)
 {
@@ -182,44 +90,19 @@  do_test (void)
   int result = 0;
   for (size_t i = 0; i < sizeof (tests) / sizeof (tests[0]); i++)
     {
-      result |= test_in_one_mode (tests[i].s, &tests[i].rn, &tests[i].exact,
-				  "default rounding mode",
-				  true, true, true);
-#ifdef FE_DOWNWARD
-      if (!fesetround (FE_DOWNWARD))
+      result |= test_in_one_mode (tests[i].s, &tests[i].r[modes[0].rnd_i],
+				  &tests[i].exact, modes[0].mode_name,
+				  modes[0].rnd_ok);
+      for (const struct fetestmodes *m = &modes[1]; m->mode_name != NULL; m++)
 	{
-	  result |= test_in_one_mode (tests[i].s, &tests[i].rd,
-				      &tests[i].exact, "FE_DOWNWARD",
-				      ROUNDING_TESTS (float, FE_DOWNWARD),
-				      ROUNDING_TESTS (double, FE_DOWNWARD),
-				      ROUNDING_TESTS (long double,
-						      FE_DOWNWARD));
-	  fesetround (save_round_mode);
+	  if (!fesetround (m->rnd_mode))
+	    {
+	      result |= test_in_one_mode (tests[i].s, &tests[i].r[m->rnd_i],
+					  &tests[i].exact, m->mode_name,
+					  m->rnd_ok);
+	      fesetround (save_round_mode);
+	    }
 	}
-#endif
-#ifdef FE_TOWARDZERO
-      if (!fesetround (FE_TOWARDZERO))
-	{
-	  result |= test_in_one_mode (tests[i].s, &tests[i].rz,
-				      &tests[i].exact, "FE_TOWARDZERO",
-				      ROUNDING_TESTS (float, FE_TOWARDZERO),
-				      ROUNDING_TESTS (double, FE_TOWARDZERO),
-				      ROUNDING_TESTS (long double,
-						      FE_TOWARDZERO));
-	  fesetround (save_round_mode);
-	}
-#endif
-#ifdef FE_UPWARD
-      if (!fesetround (FE_UPWARD))
-	{
-	  result |= test_in_one_mode (tests[i].s, &tests[i].ru,
-				      &tests[i].exact, "FE_UPWARD",
-				      ROUNDING_TESTS (float, FE_UPWARD),
-				      ROUNDING_TESTS (double, FE_UPWARD),
-				      ROUNDING_TESTS (long double, FE_UPWARD));
-	  fesetround (save_round_mode);
-	}
-#endif
     }
   return result;
 }
diff --git a/stdlib/tst-strtod.h b/stdlib/tst-strtod.h
index d53c1de..a22e2b8 100644
--- a/stdlib/tst-strtod.h
+++ b/stdlib/tst-strtod.h
@@ -19,8 +19,32 @@ 
 #ifndef _TST_STRTOD_H
 #define _TST_STRTOD_H
 
+/* Used in the RND_* TEST macros.  */
+#include <float.h>
+#include <math-tests.h>
+
 #define FSTRLENMAX 128
 
+#define RND_OKf  1
+#define RND_OKd  2
+#define RND_OKld 4
+
+#if LDBL_MANT_DIG == 106 && LDBL_MAX_EXP == 1024
+/* This is a stupid hack for IBM long double.  This test ignores
+   inexact values for long double due to the limitations of the
+   format.  This ensures testing the RND_OKld bit is always false.  */
+# define _RND_OKld 0
+#else
+# define _RND_OKld RND_OKld
+#endif
+
+#define IS_RND_OK(m)					    \
+    ((ROUNDING_TESTS (float, m) ? RND_OKf : 0)		    \
+     | (ROUNDING_TESTS (double, m) ? RND_OKd : 0)	    \
+     | (ROUNDING_TESTS (long double, m) ? _RND_OKld : 0))
+
+#define RND_OK_DFLT (RND_OKf | RND_OKd | _RND_OKld)
+
 /* Splat n variants of the same test for the various strtod functions.  */
 #define GEN_TEST_STRTOD_FOREACH(mfunc)		  \
     mfunc (  f,       float, snprintf,  "", f, f) \
@@ -35,6 +59,12 @@ 
    CSUF - C standardish suffix for many of the math functions
 */
 
+/* Declare a member element for each floating point type
+   with a suffix matching FSUF above.  */
+#define STRUCT_FOREACH_TYPE(n)	\
+  float n ## f;			\
+  double n ## d;		\
+  long double n ## ld;
 
 
 #define STRTOD_TEST_FOREACH(mfunc, ...)	  \
@@ -46,5 +76,64 @@ 
    result;				  \
 })
 
+/* Define the long double choose (LDC) macro
+   to select the appropriate generated long double
+   value from the generated test data.  */
+#if LDBL_MANT_DIG == 53 && LDBL_MAX_EXP == 1024
+/* This is for the long double == double format.  */
+# define LDC(a, ...) a
+#elif LDBL_MANT_DIG == 64 && LDBL_MAX_EXP == 16384 && LDBL_MIN_EXP == -16381
+/* This is for the Intel extended float format.  */
+# define LDC(a, b, ...) b
+#elif LDBL_MANT_DIG == 64 && LDBL_MAX_EXP == 16384 && LDBL_MIN_EXP == -16382
+/* This is for the Motorola extended float format.  */
+# define LDC(a, b, c ...) c
+#elif LDBL_MANT_DIG == 106 && LDBL_MAX_EXP == 1024
+/* This is for the IBM extended double format.  */
+# define LDC(a, b, c, d, ...) d
+#elif LDBL_MANT_DIG == 113 && LDBL_MAX_EXP == 16384
+/* This is for the IEEE binary128 format.  */
+# define LDC(a, b, c, d, e, ...) e
+#else
+# error "unknown long double format"
+#endif
+
+/* This macro is used in conjunction with the output from the
+   gen-tst-strtod-round utility to select the appropriately
+   rounded long double value for a given format.  */
+#define TEST(s,							\
+	     fx, fd, fn, fz, fu,				\
+	     dx, dd, dn, dz, du,				\
+	     ld53x, ld53d, ld53n, ld53z, ld53u,			\
+	     ld64ix, ld64id, ld64in, ld64iz, ld64iu,		\
+	     ld64mx, ld64md, ld64mn, ld64mz, ld64mu,		\
+	     ld106x, ld106d, ld106n, ld106z, ld106u,		\
+	     ld113x, ld113d, ld113n, ld113z, ld113u)		\
+  {								\
+    s,								\
+    { fx, dx, LDC (ld53x, ld64ix, ld64mx, ld106x, ld113x) },	\
+    {								\
+    { fn, dn, LDC (ld53n, ld64in, ld64mn, ld106n, ld113n) },	\
+    { fd, dd, LDC (ld53d, ld64id, ld64md, ld106d, ld113d) },	\
+    { fz, dz, LDC (ld53z, ld64iz, ld64mz, ld106z, ld113z) },	\
+    { fu, du, LDC (ld53u, ld64iu, ld64mu, ld106u, ld113u) }	\
+    }								\
+  }
+
+struct test_exactness
+  {
+  STRUCT_FOREACH_TYPE ()
+  };
+
+struct test_results
+  {
+  STRUCT_FOREACH_TYPE ()
+  };
+
+struct test {
+  const char *s;
+  struct test_exactness exact;
+  struct test_results r[4];
+};
 
 #endif