[PATCH v1 2/2] psxtests: Fix math function build warnings

[Stephen Clark]

Gedare,
Sorry, I didn't see that you had written comments below the first one yesterday. 

The conditional logic for building the long double functions comes from newlib. Given that it could change, it made sense to handle it in a single place in case it changed. As far as moving the logic into the build system, I talked with Joel and he's not sure how this would be accomplished with the build system, let alone if it would be desirable.

The license info will be added.


-----Original Message-----
From: Gedare Bloom <gedare at rtems.org> 
Sent: Thursday, March 18, 2021 10:56 AM
To: Stephen Clark <stephen.clark at oarcorp.com>
Cc: devel at rtems.org
Subject: Re: [PATCH v1 2/2] psxtests: Fix math function build warnings

Hi Stephen,

On Thu, Mar 18, 2021 at 8:19 AM Stephen Clark <stephen.clark at oarcorp.com> wrote:
>
> Added conditionals to ensure that long double function tests were only 
> built when newlib has long double math functions.

This patch is not related to the other patch in the series. I recommend that you generally should send unrelated patches separately from each other, rather than in a group like this. That makes the review easier.  For example, I can't really give a good review on patch 1/2.

> ---
>  testsuites/psxtests/psxhdrs/math/acoshl.c        |  3 +++
>  testsuites/psxtests/psxhdrs/math/acosl.c         |  3 +++

[...]

> diff --git a/testsuites/psxtests/psxhdrs/math/has_long_double.h 
> b/testsuites/psxtests/psxhdrs/math/has_long_double.h
> new file mode 100644
> index 0000000000..d813d08a61
> --- /dev/null
> +++ b/testsuites/psxtests/psxhdrs/math/has_long_double.h
> @@ -0,0 +1,16 @@
> +#ifndef HAS_LONG_DOUBLE_H
> +#define HAS_LONG_DOUBLE_H
> +//copied from newlib math.h on 21 Jan 2021

The exact location might be helpful. Since this file wasn't copied entirely, the file header block should be provided as per https://docs.rtems.org/branches/master/eng/coding-file-hdr.html


> +
> +/* Newlib doesn't fully support long double math functions so far.
> +   On platforms where long double equals double the long double functions
> +   simply call the double functions.  On Cygwin the long double functions
> +   are implemented independently from newlib to be able to use optimized
> +   assembler functions despite using the Microsoft x86_64 ABI. */ #if 
> +defined (_LDBL_EQ_DBL) || defined (__CYGWIN__)

I guess __CYGWIN__ is never defined?

> +#ifndef __math_68881
> +#define NEWLIB_HAS_LONG_DOUBLE_MATH_FUNCTIONS 1 #endif #endif
> +

This header reduces to
#if defined (_LDBL_EQ_DBL) && !defined(__math_68881) #define NEWLIB_HAS_LONG_DOUBLE_MATH_FUNCTIONS 1 #endif

but, since you just use this as a feature check anyway, you could replace all #if defined NEWLIB_HAS_LONG_DOUBLE_MATH_FUNCTIONS
with
#if defined (_LDBL_EQ_DBL) && !defined(__math_68881) /* newlib has long double */ in this patch, and you don't need this header, unless the feature check is likely to change?

> +#endif
> diff --git a/testsuites/psxtests/psxhdrs/math/hypotl.c 
> b/testsuites/psxtests/psxhdrs/math/hypotl.c
> index bffc632c09..182e4028d8 100644
> --- a/testsuites/psxtests/psxhdrs/math/hypotl.c
> +++ b/testsuites/psxtests/psxhdrs/math/hypotl.c
> @@ -35,7 +35,9 @@
>  #endif
>
>  #include <math.h>
> +#include "has_long_double.h"
>
> +#if defined NEWLIB_HAS_LONG_DOUBLE_MATH_FUNCTIONS
>  int test( void );
>
>  int test( void )
> @@ -46,3 +48,4 @@ int test( void )
>
>    return (result);
>  }
> +#endif

It seems to me that this is doing something that might better be done by the build system. Why even build these tests, if they are going to be empty? I wonder if we can probe for the long double math support in the build