This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 3/6] float128: Add strfromf128
On Fri, 26 May 2017, Gabriel F. T. Gomes wrote:
> diff --git a/NEWS b/NEWS
> index b4ecd62..8cb17cc 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -66,6 +66,9 @@ Version 2.26
> * The port to Native Client running on ARMv7-A (--host=arm-nacl) has been
> removed.
>
> +* The function strfromf128, from ISO/IEC TS 18661-3:2015, is added to libc.
> + It converts a _Float128 value into string.
Such a NEWS entry, for a function not available for all glibc
configurations, needs to indicate what systems the function is available
on. (I'd say that means there should be one NEWS entry for all the
float128 functions, added only when the powerpc64le support is enabled,
rather than piecemeal entries before then.)
> diff --git a/include/gmp.h b/include/gmp.h
> index 95d6c16..e6f635e 100644
> --- a/include/gmp.h
> +++ b/include/gmp.h
> @@ -6,6 +6,8 @@
>
> #include <stdlib/gmp.h>
>
> +#include <bits/floatn.h>
> +
> /* Now define the internal interfaces. */
> extern mp_size_t __mpn_extract_double (mp_ptr res_ptr, mp_size_t size,
> int *expt, int *is_neg,
Presumably this belongs in patch 2, which adds a test
of__HAVE_DISTINCT_FLOAT128 to this header; it seems to have nothing to do
with this patch.
> @@ -328,6 +339,52 @@ __printf_fp_l (FILE *fp, locale_t loc,
> grouping = NULL;
>
> /* Fetch the argument value. */
> +#if __HAVE_DISTINCT_FLOAT128
> + if (info->is_binary128)
I don't like this duplication of a large section of code.
As I see it, the code inside the conditional only varies between types
for: the field name (f128 in this case); the type name; the
__mpn_extract_* function called; the MANT_DIG value. (It *used* to differ
more, before we moved to using type-generic isnan / signbit / isinf macros
throughout glibc instead of direct calls to the functions those macros
might call.)
So refactor the existing code to have a macro for the code inside the
conditional, used for both the long double and double cases. Then you
just need to add a third call to the macro for the _Float128 case.
> /* Fetch the argument value. */
> +#if __HAVE_DISTINCT_FLOAT128
> + if (info->is_binary128)
The same comment about duplication applies here.
> +#if __HAVE_DISTINCT_FLOAT128
> + /* This block is copied from sysdeps/ieee754/ldbl-128/printf_fphex.c. */
> + if (info->is_binary128)
And here, the definition of PRINT_FPHEX_LONG_DOUBLE should be refactored
in some way to allow it to be shared. (It's fine for type names such as
union ieee854_float128 to be visible internally in glibc for both
binary128 long double and for _Float128, if it helps have a common macro
that can be used as-is in defining both PRINT_FPHEX_LONG_DOUBLE and e.g.
PRINT_FPHEX_FLOAT128.)
> diff --git a/stdlib/strfrom-skeleton.c b/stdlib/strfrom-skeleton.c
> index 811a29c..5841919 100644
> --- a/stdlib/strfrom-skeleton.c
> +++ b/stdlib/strfrom-skeleton.c
> @@ -132,6 +132,12 @@ STRFROM (char *dest, size_t size, const char *format, FLOAT f)
> which type of floating-point number is being passed. */
> info.is_long_double = __builtin_types_compatible_p (FLOAT, long double);
>
> + /* Similarly, the function strfromf128 passes a floating-point number in
> + _Float128 format to printf_fp. */
> +#if __HAVE_DISTINCT_FLOAT128
> + info.is_binary128 = __builtin_types_compatible_p (FLOAT, _Float128);
> +#endif
In other places you set is_binary128 even when !__HAVE_DISTINCT_FLOAT128.
It's not *used* when !__HAVE_DISTINCT_FLOAT128 - is there a deliberate
choice that in this place it shouldn't be set either?
--
Joseph S. Myers
joseph@codesourcery.com