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

[Paul E Murphy <murphyp@linux.ibm.com>]

On 10/25/19 10:34 AM, Gabriel F. T. Gomes wrote:
> From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com>
> 
> Changes since v1:
> 
>    - Adapted after patch v1 19/31 (remove hidden_* uses) was dropped.
>    - Fixed unintended removal of cvt* symbols on alpha and s390x
>      (now tested with build-many-glibcs.py).
> 
> -- 8< --
> This patch is to be squashed with the other n/5 refactoring patches.
> 
> This patch replaces the use of the APPEND macro with one new macro being
> defined for each of the cvt* functions.  This makes it easier to define
> functions names for IEEE long double on powerpc64le, e.g. __ecvtieee128.

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.

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)


> diff --git a/misc/efgcvt.c b/misc/efgcvt.c
> index 81ac60415e..93a0269d5c 100644
> --- a/misc/efgcvt.c
> +++ b/misc/efgcvt.c
> @@ -16,19 +16,27 @@
>      License along with the GNU C Library; if not, see
>      <http://www.gnu.org/licenses/>.  */
> 
> -#include "efgcvt.c"

I see some noise in the refactoring :). (and in efgcvt_r.c),


> diff --git a/misc/efgcvt_r-template.c b/misc/efgcvt_r-template.c

...

> -#define FLOOR APPEND(floor, FLOAT_NAME_EXT)
> -#define FABS APPEND(fabs, FLOAT_NAME_EXT)
> -#define LOG10 APPEND(log10, FLOAT_NAME_EXT)
> -#define EXP APPEND(exp, FLOAT_NAME_EXT)

These macros don't even appear to be used... OK.


> diff --git a/misc/efgcvt_r.c b/misc/efgcvt_r.c
> index aa0eb8ca43..5b48ca5b9b 100644
> --- a/misc/efgcvt_r.c
> +++ b/misc/efgcvt_r.c
> @@ -16,22 +16,24 @@
>      License along with the GNU C Library; if not, see
>      <http://www.gnu.org/licenses/>.  */
> 
> -#include "efgcvt_r.c"
> +#define ECVT_R ecvt_r
> +#define FCVT_R fcvt_r
> +#define __ECVT_R __ecvt_r
> +#define __FCVT_R __fcvt_r
> +#include <efgcvt-dbl-macros.h>
> +#include <efgcvt_r-template.c>
> 
>   #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0)
> -# define cvt_symbol(symbol) \
> -  cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \
> -	      APPEND (q, symbol), GLIBC_2_0); \
> -  weak_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol))
> +# define cvt_symbol(local, symbol) \
> +  cvt_symbol_1 (libc, local, APPEND (q, symbol), GLIBC_2_0); \
> +  weak_alias (local, symbol)
>   # define cvt_symbol_1(lib, local, symbol, version) \
>     libc_hidden_def (local) \
>     compat_symbol (lib, local, symbol, version)
>   #else
> -# define cvt_symbol(symbol) \
> -  cvt_symbol_1 (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol))
> -#  define cvt_symbol_1(local, symbol) \
> +# define cvt_symbol(local, symbol) \
>     libc_hidden_def (local) \
>     weak_alias (local, symbol)
>   #endif
> -cvt_symbol (fcvt_r);
> -cvt_symbol (ecvt_r);
> +cvt_symbol (__fcvt_r, fcvt_r);
> +cvt_symbol (__ecvt_r, ecvt_r);
> diff --git a/misc/qefgcvt.c b/misc/qefgcvt.c
> index ea48c6b48a..903bf93aa4 100644
> --- a/misc/qefgcvt.c
> +++ b/misc/qefgcvt.c
> @@ -16,18 +16,24 @@
>      License along with the GNU C Library; if not, see
>      <https://www.gnu.org/licenses/>.  */
> 
> -#include "efgcvt.c"
> +#define ECVT qecvt
> +#define FCVT qfcvt
> +#define GCVT qgcvt
> +#define __ECVT __qecvt
> +#define __FCVT __qfcvt
> +#define __GCVT __qgcvt
> +#define __ECVT_R __qecvt_r
> +#define __FCVT_R __qfcvt_r
> +#include <efgcvt-ldbl-macros.h>
> +#include <efgcvt-template.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) \
> -    versioned_symbol (lib, local, symbol, version)
> +# define cvt_symbol(local, symbol) \
> +  versioned_symbol (libc, local, symbol, GLIBC_2_4)
>   #else
> -# define cvt_symbol(symbol) \
> -  strong_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol))
> +# define cvt_symbol(local, symbol) \
> +  strong_alias (local, symbol)
>   #endif
> -cvt_symbol(fcvt);
> -cvt_symbol(ecvt);
> -cvt_symbol(gcvt);
> +cvt_symbol (__qfcvt, qfcvt);
> +cvt_symbol (__qecvt, qecvt);
> +cvt_symbol (__qgcvt, qgcvt);
> diff --git a/misc/qefgcvt_r.c b/misc/qefgcvt_r.c
> index d2f36e6d2a..b790bf7837 100644
> --- a/misc/qefgcvt_r.c
> +++ b/misc/qefgcvt_r.c
> @@ -17,21 +17,21 @@
>      License along with the GNU C Library; if not, see
>      <https://www.gnu.org/licenses/>.  */
> 
> -#include "efgcvt_r.c"
> +#define ECVT_R qecvt_r
> +#define FCVT_R qfcvt_r
> +#define __ECVT_R __qecvt_r
> +#define __FCVT_R __qfcvt_r
> +#include <efgcvt-ldbl-macros.h>
> +#include <efgcvt_r-template.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?

>   #else
> -# define cvt_symbol(symbol) \
> -  cvt_symbol_1 (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol))
> -#  define cvt_symbol_1(local, symbol) \
> +# define cvt_symbol(local, symbol) \
>     libc_hidden_def (local) \
>     weak_alias (local, symbol)
>   #endif

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?

> -cvt_symbol(fcvt_r);
> -cvt_symbol(ecvt_r);
> +cvt_symbol (__qfcvt_r, qfcvt_r);
> +cvt_symbol (__qecvt_r, qecvt_r);
>