Re: [PATCH] Replace use of snprintf with strfrom in libm tests

[Joseph Myers <joseph at codesourcery dot com>]

On Mon, 12 Dec 2016, Gabriel F. T. Gomes wrote:

> In order to support float128 tests, the calls to snprintf, which does
> not support the type __float128, are replaced with calls to
> strfrom{f,d,l}.

This patch appears to lose the effects of the ' ' flag, i.e. aligning 
positive and negative values in the output.

I think that alignment is desirable, meaning you need another level of 
wrappers to handle moving the output of FTOSTR and prepending it with a 
space in the case where a '-' was not output.  This is relevant both to 
the cases where you're inserting a precision in a format string, and to 
those where the precision was already embedded in the string.

Also, the PRINTF_EXPR, PRINTF_XEXPR, PRINTF_NEXPR macros only made sense 
at all in a context where snprintf was used.  With the use of strfrom, 
they don't depend on the type used, and the code would be clearer if it 
just used "e", "a", "f" directly in libm-test.inc (with the macro 
definitions and the comments about them in libm-test.inc being removed).

(Actually I think there are just four cases involved, which would indicate 
further refactoring: "% .*e" with TYPE_DECIMAL_DIG - 1 precision, "% .*a" 
with TYPE_HEX_DIG - 1 precision, "%.0f" and "% .4f".  The third of these 
should work unchanged with strfrom.  The fourth is only used with 
nonnegative values, so a space could be inserted in the users instead of 
needing special handling to emulate the ' ' flag.  But I don't think such 
further refactoring is needed for this change to go in, as long as you 
keep output unchanged and eliminate type-specific definitions of macros 
PRINTF_* when all types have the same definitions.)

> +/* The definitions TYPE_DECIMAL_DIG and TYPE_HEX_DIG are used to select the
> +   precision (i.e.: number of fractional digits) to be printed.  When using
> +   snprintf, it is possible to pass the precision in an argument with "%.*".
> +   On the other hand, strfrom does not accept such format string, thus the
> +   precision must be coded in the format string itself.  */
> +static int
> +fmt_ftostr (char *dest, size_t size, const char *format,
> +	    const char *conversion, int precision, FLOAT value)

The function comment should describe what the function does, including the 
semantics of its arguments and return value.

> +  /* Generate the format string.  */
> +  ptr_format = stpcpy(new_format, format);
> +  ret = sprintf(ptr_format, "%d", precision);
> +  ptr_format += ret;
> +  ptr_format = stpcpy(ptr_format, conversion);
> +
> +  /* Call the float to string conversion function, e.g.: strfromd.  */
> +  return FTOSTR(dest, size, new_format, value);

Missing spaces before '(', several times.

> -      FTOSTR (fstrn, FSTR_MAX, "% .*" PRINTF_EXPR, TYPE_DECIMAL_DIG - 1, f);
> -      FTOSTR (fstrx, FSTR_MAX, "% .*" PRINTF_XEXPR, TYPE_HEX_DIG - 1, f);
> +      fmt_ftostr (fstrn, FSTR_MAX, "%.", PRINTF_EXPR, TYPE_DECIMAL_DIG - 1, f);
> +      fmt_ftostr (fstrx, FSTR_MAX, "%.", PRINTF_XEXPR, TYPE_HEX_DIG - 1, f);

These are examples of cases where I think the space-padding for 
non-negative results should be preserved (possibly by making fmt_ftostr do 
that padding, if it's only ever used in cases where ' ' was used before).

You could e.g. compare test-ldouble.out (as an example of nontrivial 
output on powerpc) before and after the changes to make sure there aren't 
unexpected changes to the output of the tests.

-- 
Joseph S. Myers
joseph@codesourcery.com