Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems

[Jeff Johnston <jjohnstn at redhat dot com>]

Just a couple comments.  

1. The FORCE_EVAL macro looks for
double or long double, even though it is only used for floats in nexttowardf
By any chance was this macro in a header that you brought into
the code and if so, would it not be better to put the general-purpose
macro in local.h?  Otherwise, I would just suggest removing the double/long double
parts that are extraneous in this instance and clutter the code.

If you took it from a header, please confirm the license was the same as
nexttowardf() which is fine btw for inclusion into newlib.

2. You have put the nexttowardf function prototype in the math.h section where
_LDBL_EQ_DOUBLE is true but the code itself doesn't use the flag as
it doesn't rely on long double internals.

-- Jeff J.

----- Original Message -----
From: "Jonathan Roelofs" <jonathan@codesourcery.com>
To: "JF Bastien" <jfb@chromium.org>
Cc: "Craig Howland" <howland@lgsinnovations.com>, newlib@sourceware.org
Sent: Tuesday, December 9, 2014 9:34:37 AM
Subject: Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems

Ping


Cheers,

Jon

On 12/3/14 8:10 AM, Jonathan Roelofs wrote:
> Sigh, trying again with less context in the diff. Apparently the mailer daemon
> doesn't like big patches.
>
>
> Jon
>
> On 12/2/14 4:58 PM, Jonathan Roelofs wrote:
>> JF reminds me that I forgot to attach the patch... oops.
>>
>> Jon
>>
>> On 12/2/14 4:45 PM, Jonathan Roelofs wrote:
>>> Here's a rebased version with the broken nexttowardf replaced by the one from
>>> musl.
>>>
>>>
>>> Cheers,
>>>
>>> Jon
>>>
>>> On 12/2/14 11:55 AM, Jonathan Roelofs wrote:
>>>> JF,
>>>>
>>>> No. I've been told by one of my coworkers that nexttowardf cannot be
>>>> implemented
>>>> correctly on top of nextafterf. For that one we'll need a different
>>>> implementation, perhaps we can copy the one in musl?... I'm not sure if the
>>>> license is compatible with doing that.
>>>>
>>>> Everything in this patch aside from the nexttowardf implementation is ready to
>>>> go as far as I'm concerned. Craig, how about on your end?
>>>>
>>>>
>>>> Cheers,
>>>>
>>>> Jon
>>>>
>>>> On 12/2/14 11:45 AM, JF Bastien wrote:
>>>>> Is this change ready to land?
>>>>>
>>>>> On Wed, Sep 24, 2014 at 9:38 AM, Jonathan Roelofs
>>>>> <jonathan@codesourcery.com> wrote:
>>>>>> Craig,
>>>>>>
>>>>>> Thanks for the review!
>>>>>>
>>>>>> On 9/23/14 7:17 PM, Craig Howland wrote:
>>>>>>>
>>>>>>> On 09/23/2014 04:33 PM, Jonathan Roelofs wrote:
>>>>>>>>
>>>>>>>> Please see the attached patch.
>>>>>>>>
>>>>>>> Needs a ChangeLog entry.  Missing the patch for libm/common/Makefile.am.
>>>>>>
>>>>>> Oops. I'm not very familiar with Newlib processes, so please let me know if
>>>>>> I'm missing other things that you'd normally expect from such an upstream
>>>>>> submission.... For example, some of my co-workers reminded me to also update
>>>>>> COPYING.NEWLIB, and add copyright headers to the new files.
>>>>>>>
>>>>>>>
>>>>>>> Operationally, all looks OK, but aesthetically, the use of casting is
>>>>>>> inconsistent.  For example:
>>>>>>>
>>>>>>> +long double
>>>>>>> +nexttowardl (long double x, long double y)
>>>>>>> +{
>>>>>>> +  return nextafter((double)x, (double)y);
>>>>>>> +}
>>>>>>>
>>>>>>> The x and y arguments are cast to double to pass to nextafter(), but the
>>>>>>> return
>>>>>>> value is not cast to long double.  It works OK, but could be confusing.
>>>>>>> Are the
>>>>>>> casts intended to highlight the spots where the double/long double
>>>>>>> equality is
>>>>>>> required?
>>>>>>
>>>>>> Yeah, that was my intent.
>>>>>>>
>>>>>>> If so, it would probably be better to either cast in all places where
>>>>>>> it is theoretically needed, or to just leave them out.  The other existing
>>>>>>> functions of this style left all of them out (i.e. no casts from ld to d
>>>>>>> or d to
>>>>>>> ld)--see, for example, libm/common/nextafterl.c.  I suggest that the
>>>>>>> precedent
>>>>>>> is the better way.
>>>>>>
>>>>>> Agreed, consistency is probably better.
>>>>>>
>>>>>> Attached is a new version of this patch with these things addressed, and
>>>>>> I've added another similar shim for log2l.
>>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>>> Jon
>>>>>>>
>>>>>>>
>>>>>>> Craig
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Jon Roelofs
>>>>>> jonathan@codesourcery.com
>>>>>> CodeSourcery / Mentor Embedded
>>>>
>>>
>>
>

-- 
Jon Roelofs
jonathan@codesourcery.com
CodeSourcery / Mentor Embedded