Re: [PATCH v2 23/30] Refactor *cvt functions implementation (5/5)

["Gabriel F. T. Gomes" <gabriel at inconstante dot net dot br>]

On Thu, 14 Nov 2019, Paul E Murphy wrote:
>
>I think the non-trivial changes here to fixup the shuffling and the 
>preceding four patches should be squashed and resent as a single patch 
>to the list. I am guessing they should squash into a somewhat readable 
>patch self contained patch.

I guess you're right.  I'll send them squashed in v3.

>The slightly more substantial changes needed to support the new ldbl 
>would be more easily reviewed as a separate patch (e.g replacing the 
>__APPEND macro and reworking cvt_symbol)

Agreed.

>On 10/25/19 10:34 AM, Gabriel F. T. Gomes wrote:
>> From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com>
>>
>> -#include "efgcvt.c"  
>
>I see some noise in the refactoring :). (and in efgcvt_r.c),

:(

>>   #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0)
>> -# define cvt_symbol(symbol) \
>> -  cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \
>> -	      APPEND (FUNC_PREFIX, symbol), GLIBC_2_4)
>> -# define cvt_symbol_1(lib, local, symbol, version) \
>> +# define cvt_symbol(local, symbol) \
>>     libc_hidden_def (local) \
>> -  versioned_symbol (lib, local, symbol, version)
>> +  versioned_symbol (libc, local, symbol, GLIBC_2_4)  
>
>This isn't a bug with the patch, but should the version tested in 
>LONG_DOUBLE_COMPAT match that passed into versioned_symbol?

We test against GLIBC_2_0, because that's the version that introduced
q{efg}cvt symbols for the first time.  The LONG_DOUBLE_COMPAT macro then
expands it to SHLIB_COMPAT with GLIBC_2_0 in the 'introduced' argument and
LONG_DOUBLE_COMPAT_VERSION in the 'obsoleted' argument.

I think it would be OK to change GLIBC_2_4 to LONG_DOUBLE_COMPAT_VERSION
(maybe that would make it easier to read).

For completeness, on powerpc64, replacing GLIBC_2_0 with GLIBC_2_4 makes
test cases fail to build, e.g.:

  misc/./tst-efgcvt-template.c:214: undefined reference to `qfcvt_r'
  .../misc/tst-ldbl-efgcvt.o:(.toc+0x0): undefined reference to `qecvt'
  .../misc/tst-ldbl-efgcvt.o:(.toc+0x8): undefined reference to `qfcvt'
  .../misc/tst-ldbl-efgcvt.o:(.toc+0x10): undefined reference to `qecvt_r'
  .../misc/tst-ldbl-efgcvt.o:(.toc+0x18): undefined reference to `qfcvt_r'
  collect2: error: ld returned 1 exit status

>The two non-trivial versioning macros get duplicated in two places. Is 
>it possible to unify the four very similar instances into a single 
>shared macro?

Before this refactoring, the macro definition for {efg}cvt and q{efg}cvt
was defined in a single file, but I failed to see any benefit in it (it
relied on the LONG_DOUBLE_CVT macro and provided completely different
definitions for each case (long double and double).  Likewise for the
sharing between {efg}cvt_r and q{efg}cvt_r.

As for the differences between {efg}cvt and {efg}cvt_r, they seem too
large to allow for the sharing of code (one uses weak_symbol where the
other uses strong_symbol; only [q]{efg}cvt_r require hidden_def). Likewise
for the long double versions.

I think a single definition for all cases would make the code harder to
read (maybe it would make it easier to maintain, but I actually find it
very confusing and hard to maintain when the macros depend on too many
things).