Refactor tst-strtod-round.c for type-generic-ness
diff mbox

Message ID e35f4861-8872-f318-2a0d-78834725b4da@linux.vnet.ibm.com
State New
Headers show

Commit Message

Paul E. Murphy May 24, 2016, 8:03 p.m. UTC
I'm starting a new thread to continue discussion on the remaining
2 patches of the strtod test refactoring [1].

I've squashed them to reduce the churn, and the benefit of
splitting them is debatable.

I've left tst-strtod.h alone as the additional work needed to make
it more type-generic is thus far unique.

The final notable change is that the ldbl-64 entries are no longer
generated by gen-tst-strtod-round.  The dbl-64 entries are properly
reused by the TEST() macro for long double if necessary. 

[1] https://sourceware.org/ml/libc-alpha/2016-05/msg00531.html.

---8<---
Reduce much of the redundancy in this file, and attempt
to coral the type specific stuff to ease adding an new type.

NOTE: reviewers, you will need to build gen-tst-strtod-round.c
and regenerate tst-strtod-round-data.h.  This keeps the patch
concise.

	* stdlib/gen-tst-strtod-round.c (printfp): Remove the literal
	suffix.
	(round_str): Likewise.
	(round_for_all): Likewise, remove the now duplicate ldbl-64
	entry, and remove some magic constants.

	* stdlib/tst-strtod-round.c: (TEST): Redefine to reduce
	duplication.  Remove duplicate dbl-64 and ldbl-64 entries.
	(LDC): New macro.
	(RND_OKf): Likewise.
	(RND_OKd): Likewise.
	(RND_OKld): Likewise.
	(_RND_OKld): Likewise.
	(IS_RND_OK): Likewise.
	(RND_OK_DFLT): Likewise.
	(STRUCT_FOREACH_FLOAT_TYPE): Likewise.
	(STRUCT_FOREACH_FLOAT): Likewise.
	(test_exactness): Generate members via macro.
	(test_results): Likewise.
	(test): Update members.
	(TEST): Define once, and utilize new LDC macro to select the
	appropriate long double format for the target.
	(INFINITYf): New macro.
	(INFINITYL): Likewise.
	(fetestmodes): New structure.
	(do_test): Refactor to be type generic.
	(test_in_one_mode): Refactor duplicate code into
	(GEN_ONE_TEST): New macro.

	* stdlib/tst-strtod-round-data.h: Regenerate.
---
 stdlib/gen-tst-strtod-round.c |  46 +++----
 stdlib/tst-strtod-round.c     | 301 ++++++++++++++++++++----------------------
 2 files changed, 165 insertions(+), 182 deletions(-)

Comments

Joseph Myers May 24, 2016, 8:49 p.m. UTC | #1
On Tue, 24 May 2016, Paul E. Murphy wrote:

> I've left tst-strtod.h alone as the additional work needed to make
> it more type-generic is thus far unique.

It's not necessary for this patch, but note that GEN_TEST_STRTOD_FOREACH, 
despite its name and comment, could actually be used as a general facility 
for "generate some text for each floating-point type in succession".  It 
could be used in the definition of STRUCT_FOREACH_FLOAT if you wanted, for 
example (maybe it would need to take variable arguments and pass them 
through to mfunc) - and in that of STRTOD_TEST_FOREACH, and others.  It's 
the pieces relating to having expected results for every format, and 
related to IBM long double being special, that are harder to do in a 
generic way like that.

In any case, I think defining RND_* macros for each type, and IS_RND_OK, 
_RND_OKld etc., is making things overly complicated and increasing the 
number of bits of per-type code needed unnecessarily.  Suppose you passed 
the mode to test_in_one_mode.  Then you can use ROUNDING_TESTS (FTYPE, 
mode) in GEN_ONE_TEST to test whether rounding works for the type being 
tested, rather than calling it in do_test (as in the checked-in code) or 
in data initializers (as in your present code).  The only reason 
ROUNDING_TESTS might not be suitable is the case of IBM long double - but 
the obvious way to address that is a conditional #undef and #define of 
ROUNDING_TESTS_long_double in the IBM long double case.

> +/* Declare a member element for each floating point type
> +   with a suffix matching FSUF above.  */

Saying "above" seems wrong when FSUF is now commented in tst-strtod.h, not 
in this file.

> +/* Declare a member element for each floating point type
> +   of type with a suffix matching FSUF above.  */

Likewise.

> +/* 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.  Note that
> +   the literal suffix must be appended before the token is
> +   potentially expanded, so we can fixup such mangling later on.  */
> +#define TEST(s,							\
> +	     fx, fd, fn, fz, fu,				\
> +	     dx, dd, dn, dz, du,				\
> +	     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) },	\

I think you mean dx as the first argument of LDC here.

Patch
diff mbox

diff --git a/stdlib/gen-tst-strtod-round.c b/stdlib/gen-tst-strtod-round.c
index fa5562e..8d46610 100644
--- a/stdlib/gen-tst-strtod-round.c
+++ b/stdlib/gen-tst-strtod-round.c
@@ -60,19 +60,18 @@  string_to_fp (mpfr_t f, const char *s, mpfr_rnd_t rnd)
 #endif
 }
 
-static void
-print_fp (FILE *fout, mpfr_t f, const char *suffix, const char *suffix2)
+void
+print_fp (FILE *fout, mpfr_t f, const char *suffix)
 {
   if (mpfr_inf_p (f))
-    mpfr_fprintf (fout, "\t%sINFINITY%s", mpfr_signbit (f) ? "-" : "",
-		  suffix2);
+    mpfr_fprintf (fout, "\t%sINFINITY%s", mpfr_signbit (f) ? "-" : "", suffix);
   else
-    mpfr_fprintf (fout, "\t%Ra%s%s", f, suffix, suffix2);
+    mpfr_fprintf (fout, "\t%Ra%s", f, suffix);
 }
 
 static void
-round_str (FILE *fout, const char *s, const char *suffix,
-	   int prec, int emin, int emax, bool ibm_ld)
+round_str (FILE *fout, const char *s, int prec, int emin, int emax,
+	   bool ibm_ld)
 {
   mpfr_t f;
   mpfr_set_default_prec (prec);
@@ -94,13 +93,13 @@  round_str (FILE *fout, const char *s, const char *suffix,
       mpfr_clear (max_value);
     }
   mpfr_fprintf (fout, "\t%s,\n", r ? "false" : "true");
-  print_fp (fout, f, suffix, ",\n");
+  print_fp (fout, f, ",\n");
   string_to_fp (f, s, MPFR_RNDN);
-  print_fp (fout, f, suffix, ",\n");
+  print_fp (fout, f, ",\n");
   string_to_fp (f, s, MPFR_RNDZ);
-  print_fp (fout, f, suffix, ",\n");
+  print_fp (fout, f, ",\n");
   string_to_fp (f, s, MPFR_RNDU);
-  print_fp (fout, f, suffix, "");
+  print_fp (fout, f, "");
   mpfr_clear (f);
 }
 
@@ -108,21 +107,19 @@  static void
 round_for_all (FILE *fout, const char *s)
 {
   static const struct fmt {
-    const char *suffix;
     int prec;
     int emin;
     int emax;
     bool ibm_ld;
-  } formats[7] = {
-    { "f", 24, -148, 128, false },
-    { "", 53, -1073, 1024, false },
-    { "L", 53, -1073, 1024, false },
+  } formats[] = {
+    { 24, -148, 128, false },
+    { 53, -1073, 1024, false },
     /* This is the Intel extended float format.  */
-    { "L", 64, -16444, 16384, false },
+    { 64, -16444, 16384, false },
     /* This is the Motorola extended float format.  */
-    { "L", 64, -16445, 16384, false },
-    { "L", 106, -1073, 1024, true },
-    { "L", 113, -16493, 16384, false },
+    { 64, -16445, 16384, false },
+    { 106, -1073, 1024, true },
+    { 113, -16493, 16384, false },
   };
   mpfr_fprintf (fout, "  TEST (\"");
   const char *p;
@@ -134,11 +131,12 @@  round_for_all (FILE *fout, const char *s)
     }
   mpfr_fprintf (fout, "\",\n");
   int i;
-  for (i = 0; i < 7; i++)
+  int n_formats = sizeof (formats) / sizeof (formats[0]);
+  for (i = 0; i < n_formats; i++)
     {
-      round_str (fout, s, formats[i].suffix, formats[i].prec,
-		 formats[i].emin, formats[i].emax, formats[i].ibm_ld);
-      if (i < 6)
+      round_str (fout, s, formats[i].prec, formats[i].emin,
+		 formats[i].emax, formats[i].ibm_ld);
+      if (i < n_formats - 1)
 	mpfr_fprintf (fout, ",\n");
     }
   mpfr_fprintf (fout, "),\n");
diff --git a/stdlib/tst-strtod-round.c b/stdlib/tst-strtod-round.c
index a2cd2ab..0c7723a 100644
--- a/stdlib/tst-strtod-round.c
+++ b/stdlib/tst-strtod-round.c
@@ -29,152 +29,162 @@ 
 #include <string.h>
 #include <math-tests.h>
 
-struct exactness
-{
-  bool f;
-  bool d;
-  bool ld;
-};
+#include "tst-strtod.h"
 
-struct test_results {
-  float f;
-  double d;
-  long double ld;
-};
+#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)
 
-struct test {
-  const char *s;
-  struct exactness exact;
-  struct test_results rd, rn, rz, ru;
-};
 
+/* Declare a member element for each floating point type
+   with a suffix matching FSUF above.  */
+#define STRUCT_FOREACH_FLOAT(n)	\
+  float n ## f;			\
+  double n ## d;		\
+  long double n ## ld;
+
+/* Declare a member element for each floating point type
+   of type with a suffix matching FSUF above.  */
+#define STRUCT_FOREACH_FLOAT_TYPE(type, n)  \
+  type n ## f;				    \
+  type n ## d;				    \
+  type n ## ld;
+
+/* 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
-# 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 }							\
-  }
+/* 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 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 }							\
-  }
+# 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 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 }							\
-  }
+# define LDC(a, b, c ...) c
 #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 }							\
-  }
+/* This is for the IBM extended double format.  */
+# define LDC(a, b, c, d, ...) d
 #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 }							\
-  }
+/* 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.  Note that
+   the literal suffix must be appended before the token is
+   potentially expanded, so we can fixup such mangling later on.  */
+#define TEST(s,							\
+	     fx, fd, fn, fz, fu,				\
+	     dx, dd, dn, dz, du,				\
+	     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 ## f, dn, LDC (dn ## L, ld64in ## L, ld64mn ## L, ld106n ## L, ld113n ## L)},\
+    { fd ## f, dd, LDC (dd ## L, ld64id ## L, ld64md ## L, ld106d ## L, ld113d ## L)},\
+    { fz ## f, dz, LDC (dz ## L, ld64iz ## L, ld64mz ## L, ld106z ## L, ld113z ## L)},\
+    { fu ## f, du, LDC (du ## L, ld64iu ## L, ld64mu ## L, ld106u ## L, ld113u ## L)} \
+    }								\
+  }
+
+/* Note that the above will mangle constants generated by
+   the generator program.  We fix those up below.  */
+#define INFINITYf INFINITY
+#define INFINITYL INFINITY
+
+struct test_exactness
+  {
+  STRUCT_FOREACH_FLOAT_TYPE (bool,)
+  };
+
+struct test_results
+  {
+  STRUCT_FOREACH_FLOAT ()
+  };
+
+struct test {
+  const char *s;
+  struct test_exactness exact;
+  struct test_results r[4];
+};
+
 /* 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 +192,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;
 }