Message ID | 20150502164728.GA3618@redhat.com |
---|---|
State | New |
Headers | show |
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.
diff --git a/libstdc++-v3/include/experimental/numeric b/libstdc++-v3/include/experimental/numeric index a11516b..b284110 100644 --- a/libstdc++-v3/include/experimental/numeric +++ b/libstdc++-v3/include/experimental/numeric @@ -40,7 +40,6 @@ #else #include <experimental/type_traits> -#include <cmath> namespace std _GLIBCXX_VISIBILITY(default) { @@ -52,7 +51,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #define __cpp_lib_experimental_gcd_lcm 201411 - // Greatest common divisor + /// Greatest common divisor template<typename _Mn, typename _Nn> constexpr common_type_t<_Mn, _Nn> gcd(_Mn __m, _Nn __n) @@ -60,12 +59,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static_assert(is_integral<_Mn>::value, "arguments to gcd are integers"); static_assert(is_integral<_Nn>::value, "arguments to gcd are integers"); - return __m == 0 ? std::abs(__n) - : __n == 0 ? std::abs(__m) - : fundamentals_v2::gcd(__n, __m % __n); + return __m == 0 ? __builtin_abs(__n) + : __n == 0 ? __builtin_abs(__m) + : gcd(__n, __m % __n); } - // Least common multiple + /// Least common multiple template<typename _Mn, typename _Nn> constexpr common_type_t<_Mn, _Nn> lcm(_Mn __m, _Nn __n) @@ -74,7 +73,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static_assert(is_integral<_Nn>::value, "arguments to lcm are integers"); return (__m != 0 && __n != 0) - ? (std::abs(__m) / fundamentals_v2::gcd(__m, __n)) * std::abs(__n) + ? (__builtin_abs(__m) / gcd(__m, __n)) * __builtin_abs(__n) : 0; }