Re: [PATCH] Add tst-wcstod-round

[Carlos O'Donell <carlos at redhat dot com>]

On 08/04/2016 10:50 AM, Paul E. Murphy wrote:
> This extends tst-strtod-round with a few trivial changes
> to also test the wide character variants of strto* using
> similar macros to other shared tests.
> 
> 	* stdlib/tst-strtod-round.c:
> 	(L_): New macro to select string encoding.
> 	(FNPFX): New macro to select str or wcs prefix.
> 	(CHAR): New macro to choose wchar_t or char.
> 	(STRM): New macro to choose printf modifier for above.
> 	(STRTO): New macro to choose appropriate string -> real
> 	function.
> 	(TEST_WIDE): Conditional definition to enable wchar_t
> 	testing.
> 	(FNPFXS): "wcs" or "str" dependent on [TEST_WIDE].
> 	(STR): Support for above macro.
> 	(STRX): Likewise.
> 
> 	(TEST): Update with above macros.
> 	(test): Likewise.
> 	(GEN_ONE_TEST): Likewise.
> 	(test_in_one_mode): Likewise.
> 
> 	* wcsmbs/tst-wcstod-round.c: New file.
> 
> 	* wcsmbs/Makefile: (tests): Add tst-wcstod-round
> 	(tst-wcstod-round): Add libm depencency for fesetround.

At a high-level the patch is perfect.

At a implementation level I'd like to see a little more reorg
to avoid macro API typos.

See below. Otherwise this looks good to me.

> ---
>  stdlib/tst-strtod-round.c | 43 +++++++++++++++++++++++++++++++++----------
>  wcsmbs/Makefile           |  3 +++
>  wcsmbs/tst-wcstod-round.c |  3 +++
>  3 files changed, 39 insertions(+), 10 deletions(-)
>  create mode 100644 wcsmbs/tst-wcstod-round.c
> 
> diff --git a/stdlib/tst-strtod-round.c b/stdlib/tst-strtod-round.c
> index 509f96a..023d382 100644
> --- a/stdlib/tst-strtod-round.c
> +++ b/stdlib/tst-strtod-round.c
> @@ -31,9 +31,26 @@
>  
>  #include "tst-strtod.h"
>  
> +/* Build the test for wide or normal character strings.  */
> +#ifdef TEST_WIDE

You should always define TEST_WIDE and set it either to 0 or 1,
but instead I suggest removing it and doing what I suggest below.

You should still read this:
https://sourceware.org/glibc/wiki/Wundef

> +# define L_(str) L ## str
> +# define FNPFX wcs
> +# define CHAR wchar_t
> +# define STRM "%S"
> +# define snprintf swprintf
> +# define strfromf128 wcsfromf128

This above block of defines should live in tst-wcstod-round.c since
that's the code that needs them, like other similar tests.

> +#else
> +# define L_(str) str
> +# define FNPFX str
> +# define CHAR char
> +# define STRM "%s"

These are the defaults macro API values and should always bet set.
I suggest moving this file into a generic test source e.g.
tst-strtox-round.c and then have tst-wcstod-round define the above
defaults and include the generic version.

This avoid macro typos which result in the wrong thing being
tested.

> +#endif
> +
>  #define _CONCAT(a, b) a ## b
>  #define CONCAT(a, b) _CONCAT (a, b)
>  
> +#define STRTO(x) CONCAT (CONCAT (FNPFX, to), x)
> +
>  #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
> @@ -110,7 +127,7 @@
>  	     ld106x, ld106d, ld106n, ld106z, ld106u,	\
>  	     ld113x, ld113d, ld113n, ld113z, ld113u)	\
>    {							\
> -    s,							\
> +    L_ (s),						\
>      { XNTRY (fx, dx, ld64ix, ld64mx, ld106x, ld113x) },	\
>      {							\
>      { ENTRY (fn, dn, ld64in, ld64mn, ld106n, ld113n) },	\
> @@ -131,7 +148,7 @@ struct test_results
>    };
>  
>  struct test {
> -  const char *s;
> +  const CHAR *s;
>    struct test_exactness exact;
>    struct test_results r[4];
>  };
> @@ -139,19 +156,25 @@ struct test {
>  /* Include the generated test data.  */
>  #include "tst-strtod-round-data.h"
>  
> +#define STRX(x) #x
> +#define STR(x) STRX (x)
> +#define FNPFXS STR (FNPFX)
> +
>  #define GEN_ONE_TEST(FSUF, FTYPE, FTOSTR, FTOSTRM, LSUF, CSUF)	\
>  {								\
> -  FTYPE f = strto ## FSUF (s, NULL);				\
> +  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);		\
> +      CHAR efstr[FSTRLENMAX];					\
> +      CHAR fstr[FSTRLENMAX];					\
> +      FTOSTR (efstr, FSTRLENMAX, L_("%") L_(FTOSTRM) L_("a"),   \
> +	      expected->FSUF);    				\
> +      FTOSTR (fstr, FSTRLENMAX, L_("%") L_(FTOSTRM) L_("a"), f);\
> +      printf (FNPFXS "to" #FSUF  " (" STRM ") returned " STRM   \
> +	      " not " STRM " (%s)\n",				\
> +	      s, fstr, efstr, mode_name);			\
>        if (ROUNDING_TESTS (FTYPE, rnd_mode) || exact->FSUF)	\
>  	result = 1;						\
>        else							\
> @@ -160,7 +183,7 @@ struct test {
>  }
>  
>  static int
> -test_in_one_mode (const char *s, const struct test_results *expected,
> +test_in_one_mode (const CHAR *s, const struct test_results *expected,
>  		  const struct test_exactness *exact, const char *mode_name,
>  		  int rnd_mode)
>  {
> diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile
> index 8b599f7..9384a10 100644
> --- a/wcsmbs/Makefile
> +++ b/wcsmbs/Makefile
> @@ -49,6 +49,7 @@ strop-tests :=  wcscmp wcsncmp wmemcmp wcslen wcschr wcsrchr wcscpy wcsnlen \
>  tests := tst-wcstof wcsmbs-tst1 tst-wcsnlen tst-btowc tst-mbrtowc \
>  	 tst-wcrtomb tst-wcpncpy tst-mbsrtowcs tst-wchar-h tst-mbrtowc2 \
>  	 tst-c16c32-1 wcsatcliff tst-wcstol-locale tst-wcstod-nan-locale \
> +	 tst-wcstod-round \
>  	 $(addprefix test-,$(strop-tests))
>  
>  include ../Rules
> @@ -68,6 +69,8 @@ $(objpfx)tst-wcstol-locale.out: $(gen-locales)
>  $(objpfx)tst-wcstod-nan-locale.out: $(gen-locales)
>  endif
>  
> +$(objpfx)tst-wcstod-round: $(libm)
> +
>  CFLAGS-wcwidth.c = -I../wctype
>  CFLAGS-wcswidth.c = -I../wctype
>  
> diff --git a/wcsmbs/tst-wcstod-round.c b/wcsmbs/tst-wcstod-round.c
> new file mode 100644
> index 0000000..474b235
> --- /dev/null
> +++ b/wcsmbs/tst-wcstod-round.c
> @@ -0,0 +1,3 @@
> +#include <wchar.h>
> +#define TEST_WIDE
> +#include <stdlib/tst-strtod-round.c>
> 


-- 
Cheers,
Carlos.