This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC PATCH v2 1/3] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
On Tue, 5 Jun 2018, Gabriel F. T. Gomes wrote:
> +#define LDBL_USES_FLOAT128 ((mode_flags && PRINTF_LDBL_USES_FLOAT128) == 1)
That && needs to be &, at which point you need to test != 0 rather than ==
1.
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +# define SETUP_FLOAT128_INFO(INFO) \
> + if (LDBL_USES_FLOAT128) \
> + INFO.is_binary128 = 1; \
> + else \
> + INFO.is_binary128 = 0;
> +#else
> +# define SETUP_FLOAT128_INFO(INFO) \
> + INFO.is_binary128 = 0;
> +#endif
I think the first case here is wrong. As I understand it, this macro is
used in the case of positional parameters or registered printf handlers,
where the string is first parsed (via printf-parsemb.c, which has no
special handling of different long double formats), and then the arguments
are extracted in order, and then the format specifiers are processed with
the arguments that were extracted. In particular, the code using it
> @@ -789,8 +821,7 @@ static const uint8_t jump_table[] =
> fspec->data_arg_type = PA_DOUBLE; \
> fspec->info.is_long_double = 0; \
> } \
> - /* Not supported by *printf functions. */ \
> - fspec->info.is_binary128 = 0; \
> + SETUP_FLOAT128_INFO (fspec->info) \
> \
> function_done = __printf_fp (s, &fspec->info, &ptr); \
> } \
(and likewise for hex floats) executes for a double argument, as well as
long double. So what I think you actually need to do is copy the
is_long_double setting to is_binary128 in the LDBL_USES_FLOAT128 case,
rather than setting it to 1 unconditionally.
This illustrates the need for more thorough tests of vfprintf, which
should test double arguments as well as long double, and test the use of
positional arguments. (Given such thorough tests for one printf-family
function, other printf-family functions could then have less detailed
tests that suffice to verify the correct long double format gets handled.)
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> + else if (LDBL_USES_FLOAT128)
> + args_value[cnt].pa_float128 = va_arg (*ap_savep, __float128);
> +#endif
I think you should generally be using _Float128, not __float128, in such
code.
--
Joseph S. Myers
joseph@codesourcery.com