This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Add tst-wcstod-round
- From: Carlos O'Donell <carlos at redhat dot com>
- To: "Paul E. Murphy" <murphyp at linux dot vnet dot ibm dot com>, "libc-alpha at sourceware dot org" <libc-alpha at sourceware dot org>
- Cc: Joseph Myers <joseph at codesourcery dot com>
- Date: Thu, 4 Aug 2016 12:17:13 -0400
- Subject: Re: [PATCH] Add tst-wcstod-round
- Authentication-results: sourceware.org; auth=none
- References: <70c6242f-931e-56e7-e719-32d063303f00@linux.vnet.ibm.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.