std::experimental::gcd and std::experimental::lcd

On 02/05/15 18:27 +0200, Marc Glisse wrote:
>On Sat, 2 May 2015, Jonathan Wakely wrote:
>
>>These where simple to implement (almost too simple ... I probably
>>got something wrong!)
>
>I didn't remember that std::abs works for unsigned. It will need more 
>work for performance, but that can certainly be done later (I didn't 
>look at the code beyond checking what you meant by "simple").

std::abs seems to work fine for unsigned, the overload in <cmath> for
integral types just uses __builtin_fabs. Maybe it would be better for
gcd() to just use that directly instead of including <cmath> (as
attached, which also removes the qualification on the call to gcd
because the functions only work for integral types which have no
associated namespaces anyway).

>>(Apart from using common_type_t, which is easy to change, these
>>functions meet the simpler rules for C++11 constexpr, so moving them
>>out of <experimental/numeric> would probably allow <ratio> to be
>>greatly simplified. I don't plan on doing that myself any time soon,
>>but it would make sense to do it some day.)
>
>gcd is not really the hard part in ratio. But constexpr should help, 
>it made sense not to have it in tr1, but I don't remember why we 
>didn't use it in the more recent changes (2011, the compiler probably 
>already supported constexpr). Maybe the interesting functions were too 
>hard to write as one-liners...

<ratio> was added to libstdc++ in 2008 so I think the constexpr
support was not good enough at the time.

On Sat, 2 May 2015, Jonathan Wakely wrote:

> On 02/05/15 18:27 +0200, Marc Glisse wrote:
>> On Sat, 2 May 2015, Jonathan Wakely wrote:
>> 
>>> These where simple to implement (almost too simple ... I probably
>>> got something wrong!)
>> 
>> I didn't remember that std::abs works for unsigned. It will need more work 
>> for performance, but that can certainly be done later (I didn't look at the 
>> code beyond checking what you meant by "simple").
>
> std::abs seems to work fine for unsigned, the overload in <cmath> for
> integral types just uses __builtin_fabs.

That's bad! You don't want to go through floating point, that cannot even 
represent int64_t accurately.

> Maybe it would be better for
> gcd() to just use that directly instead of including <cmath> (as
> attached, which also removes the qualification on the call to gcd
> because the functions only work for integral types which have no
> associated namespaces anyway).

Actually you use __builtin_abs (no 'f') in the patch, which apparently 
casts to int, that's even worse...

> <ratio> was added to libstdc++ in 2008 so I think the constexpr
> support was not good enough at the time.

Yes, but we modified a few pieces with Paolo in 2011, and those could have 
used constexpr I guess.

On Sat, 2 May 2015, Jonathan Wakely wrote:

> std::abs seems to work fine for unsigned, the overload in <cmath> for
> integral types just uses __builtin_fabs.

http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#2192
That overload should die.

On 02/05/15 19:07 +0200, Marc Glisse wrote:
>On Sat, 2 May 2015, Jonathan Wakely wrote:
>
>>On 02/05/15 18:27 +0200, Marc Glisse wrote:
>>>On Sat, 2 May 2015, Jonathan Wakely wrote:
>>>
>>>>These where simple to implement (almost too simple ... I probably
>>>>got something wrong!)
>>>
>>>I didn't remember that std::abs works for unsigned. It will need 
>>>more work for performance, but that can certainly be done later (I 
>>>didn't look at the code beyond checking what you meant by 
>>>"simple").
>>
>>std::abs seems to work fine for unsigned, the overload in <cmath> for
>>integral types just uses __builtin_fabs.
>
>That's bad! You don't want to go through floating point, that cannot 
>even represent int64_t accurately.

Good point - we should fix <cmath> then!

>>Maybe it would be better for
>>gcd() to just use that directly instead of including <cmath> (as
>>attached, which also removes the qualification on the call to gcd
>>because the functions only work for integral types which have no
>>associated namespaces anyway).
>
>Actually you use __builtin_abs (no 'f') in the patch, which apparently 
>casts to int, that's even worse...

Oops, that's a typo (I didn't even test that change).

>><ratio> was added to libstdc++ in 2008 so I think the constexpr
>>support was not good enough at the time.
>
>Yes, but we modified a few pieces with Paolo in 2011, and those could 
>have used constexpr I guess.

Ah I see. What we have now works, so it's not a priority to try and
replace bits of it with constexpr functions.