This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]