Re: [PATCH v4] Add nextup and nextdown math functions

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 14 Jun 2016, Rajalakshmi Srinivasaraghavan wrote:

> +#if TEST_COND_m68k96
> +    TEST_ff_f (nextafter, -0x0.fffffffep-16383L, 0.0L, -0x3.fffffff7fffffff0p-16385L, INEXACT_EXCEPTION|UNDERFLOW_EXCEPTION|ERRNO_ERANGE),
>  #endif

I think the test you have there is wrong for m68k96.  For intel96, the 
smallest normal exponent is -16382; for m68k96, the smallest normal 
exponent is -16383.  This input has exponent -16384.  So for m68k96 it's a 
63-bit mantissa.  The input has bits <31 1s><32 0s>.  So the correct 
output has bits <30 1s><0><32 1s>.  But your output has bits <30 1s><0><31 
1s>.  That is, the final digit in your output should be 8, not 0.

In general, it seems very confusing in such tests to show the exponent 
differently in the output from the input.  Why do you show the input with 
exponent -16383 but the output with exponent -16385?  That applies to 
other tests in this patch as well.

> +    TEST_f_f (nextup, 0, min_subnorm_value, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_f_f (nextup, plus_zero, min_subnorm_value, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),

0 and plus_zero are the same thing.  These are duplicate tests.

> +    TEST_f_f (nextup, snan_value, qnan_value, INVALID_EXCEPTION),
> +    TEST_f_f (nextup, -snan_value, qnan_value, INVALID_EXCEPTION),

As exactly defined functions, it should be 
NO_INEXACT_EXCEPTION|INVALID_EXCEPTION for the exception expectations for 
nextup and nextdown on sNaN input (whereas for functions that aren't 
exactly defined, Annex F says nothing about "inexact" for sNaN inputs, 
although no exceptions are allowed for qNaN inputs).

> +#if TEST_COND_m68k96
> +    TEST_f_f (nextup, -0x0.fffffffep-16383L, -0x3.fffffff7fffffff0p-16385L, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +#endif

Same issue as with the nextafter test.

> +    TEST_f_f (nextdown, 0, -min_subnorm_value, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_f_f (nextdown, plus_zero, -min_subnorm_value, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),

Same point about duplicate tests.

> +    TEST_f_f (nextdown, snan_value, qnan_value, INVALID_EXCEPTION),
> +    TEST_f_f (nextdown, -snan_value, qnan_value, INVALID_EXCEPTION),

Same point about exception expectations.

> +#if TEST_COND_m68k96
> +    TEST_f_f (nextdown, -0x3.fffffff7fffffff0p-16385L, -0x0.fffffffep-16383L, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +#endif

Same point about correct m68k96 values.

> +#ifdef NO_LONG_DOUBLE
> +  strong_alias (__nextdown, __nextdownl)
> +  weak_alias (__nextdown, nextdownl)
> +#endif

These *_alias calls aren't normally indented.

> +  if ((ix | lx) == 0)
> +    {
> +      INSERT_WORDS (x, 0, 1);	/* Return minsubnormal.  */
> +      return x;

Would seem simpler just to do "return DBL_TRUE_MIN;", having included 
<float.h> (and then no comment is needed, since the plain meaning of the 
code says what it does).

> +#ifdef NO_LONG_DOUBLE
> +  strong_alias (__nextup, __nextupl)
> +  weak_alias (__nextup, nextupl)
> +#endif

Same comment about indentation.

> +  if (ix == 0)
> +    {
> +      SET_FLOAT_WORD (x, 1);	/* Return minsubnormal.  */
> +      return x;

Return FLT_TRUE_MIN.

> +  if ((ix | lx) == 0)
> +    {
> +      /* Return minsubnormal.  */
> +      SET_LDOUBLE_WORDS64 (x, 0, 1);

Return LDBL_TRUE_MIN.

> +  if (ihx == 0)
> +    {
> +      hx =  1;
> +      INSERT_WORDS64 (xhi, hx);
> +      x = xhi;
> +      return x;

Likewise.

> +  if ((ix | hx | lx) == 0)
> +    {
> +      SET_LDOUBLE_WORDS (x, 0, 0, 1);	/* Return minsubnormal.  */
> +      return x;
> +    }

Likewise.

-- 
Joseph S. Myers
joseph@codesourcery.com