Re: [PATCH v4] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 16/05/2019 18:36, Gabriel F. T. Gomes wrote:
> On Thu, May 09 2019, Adhemerval Zanella wrote:
>>
>> On 25/02/2019 10:39, Gabriel F. T. Gomes wrote:
>>>
>>> Tested for powerpc64le (with additional patches to actually enable the
>>> use of these preparations) and for x86_64.
>>
>> It at least shows no issue when applied against master, although current 
>> tests not stress the code itself. And it seems that the code would only be 
>> enabled for powerpc with -mabi=ieeelongdouble, so I would if it would be 
>> better to actually push this patch along with powerpc64 -mabi=ieeelongdouble
>> patchset.
> 
> Well, I intentionally write small patches that are valid for master, so
> they can be applied sooner than later.  I have written many patches this
> way to avoid the burden of carrying patches on personal branches.
> 
> I also provided a link to such branch where the changes in this patch
> get tested for many of the functions added by subsequent patches.
> 
> I was hoping that these things combined would provide a good
> justification for acceptance.  With that in mind, do you have an
> objection to this patch?

My main objection is currently glibc still does not support -mabi=ieeelongdouble
(correct me if I wrong), so it might take some time to actually test this
code path on powerpc. 

> 
>>> +   PRINTF_LDBL_USES_FLOAT128 is used on platforms where the long double
>>> +   format used to be different from the IEC 60559 double format *and*
>>> +   also different from the Quadruple 128-bits IEC 60559 format (such as
>>> +   the IBM Extended Precision format on powerpc or the 80-bits IEC 60559
>>> +   format on x86), but was later converted to the Quadruple 128-bits IEC
>>> +   60559 format, which is the same format that the _Float128 always has
>>> +   (hence the `USES_FLOAT128' suffix in the name of the flag).  When set
>>> +   to one, this macros indicates that long double values are to be
>>
>> s/macros/macro
> 
> Thanks.  Fixed locally.
> 
>>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>>> +# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO)				      \
>>> [...]
>>
>> Do we really to continue use the macro-style to extend this code? I usually
>> think is is more readable to just use static inline function instead, 
>> something as:
> 
> Sounds good to me.  I'll send a new version.
> 
>> static void
>> parse_float_va_arg_extended (struct printf_info *info,
>>                              union printf_arg *the_arg, int is_long_double,
>>                              unsigned int mode_flags, va_list *ap)
> 
> Should I also add an __always_inline attribute?

Do we need it for correctness or does it show in any performance evaluation?
Otherwise I just let compiler handle it.

> 
>>> +#define PARSE_FLOAT_VA_ARG(INFO)					      \
>>> [...]
>>
>> Same as before.
> 
> OK.
>