Re: [PATCH] Add nextup and nextdown math functions

[Joseph Myers <joseph at codesourcery dot com>]

On Wed, 1 Jun 2016, Rajalakshmi Srinivasaraghavan wrote:

> This patch adds nextup and nextdown functions from TS 18661-1 before
> enabling float128 support.

I think the description for the commit message should expand more on the 
change and its rationale.

> +* TS 18661 adds nextup and nextdown functions alongside nextafter to
> +  provide support for float128 equivalent to it.  So nextup and nextdown
> +  functions are added as preparatory patches before Float128 support. This
> +  is currently enabled as a GNU extension.

But I don't think this is what a NEWS entry should look like.  The 
rationale for adding these particular functions in isolation is much less 
relevant to the NEWS entry.  Rather, it should say what functions are 
added (six of them, not two), where they come from, what (briefly) they do 
and that they are currently handled as a GNU extension (i.e. require 
_GNU_SOURCE).  Cf. the NEWS entry for the issignaling macro - the 
motivation for adding that in isolation was for use in glibc tests (we 
needed some functionality, there was a defined API for it to it made sense 
to provide that API for use outside glibc), but that motivation is also 
not relevant to people interested in what's new in a glibc version.

> +/* Copyright (C) 2016 Free Software Foundation, Inc.

New files should start with a description of the file *before* the 
copyright notice.

> +/* nextdown(x) returns the least floating-point number in the
> +   format of x less than x.  */

Use the GNU standard formatting for comments on function definitions, 
immediately above the function definition.

> +#include <math_private.h>
> +#include <float.h>

Don't include these where not needed.

> +	    INSERT_WORDS(x,(1&0x80000000),1);    /* return +-minsubnormal */

This "(1&0x80000000)", and similar for other formats, makes no sense.  In 
this case, you simply mean "0".  (The point being that nextup of -0 is the 
least positive subnormal, but that is different from what you'd get by the 
simple integer decrement of the representation that works for all other 
non-NaN inputs where the sign is negative.)

> +	    /* return +-minsubnormal */
> +	    SET_LDOUBLE_WORDS64(x,-1&0x8000000000000000ULL,1);

"-1&0x8000000000000000ULL" in the ldbl-128 version actually seems wrong.

> diff --git a/sysdeps/ieee754/ldbl-96/s_nextupl.c b/sysdeps/ieee754/ldbl-96/s_nextupl.c

> +	        if (hx==0 || (esx == 0 && hx == 0x80000000)) {
> +	            esx += 1;
> +	            hx |= 0x80000000;

I'm not sure the special handling of (esx == 0 && hx == 0x80000000) is 
correct for the m68k variant of ldbl-96 (LDBL_MIN_EXP == -16382); exponent 
0 acts more like a normal exponent there so I think that bit of special 
handling should be disabled in that case.

-- 
Joseph S. Myers
joseph@codesourcery.com