This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- From: Jeff Johnston <jjohnstn at redhat dot com>
- To: Jonathan Roelofs <jonathan at codesourcery dot com>
- Cc: JF Bastien <jfb at chromium dot org>, Craig Howland <howland at lgsinnovations dot com>, newlib at sourceware dot org
- Date: Tue, 9 Dec 2014 17:15:22 -0500 (EST)
- Subject: Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- Authentication-results: sourceware.org; auth=none
- References: <5421D931 dot 905 at codesourcery dot com> <5422F390 dot 1060401 at codesourcery dot com> <CABdywOc0LmUW=x6X1LQrpUkj201RYCqQNhPzz3nFfsf+X5oUJw at mail dot gmail dot com> <547E0B28 dot 1030407 at codesourcery dot com> <547E4F32 dot 6000201 at codesourcery dot com> <547E523B dot 6060703 at codesourcery dot com> <547F27F4 dot 2000007 at codesourcery dot com> <5487087D dot 60109 at codesourcery 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
- References:
- [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems
- Re: [libcxx+newlib] nexttoward{f,l,} shims for _LDBL_EQ_DBL systems