Uniform logic has or not-has GCD/LCM #676
Conversation
| #ifndef BOOST_MP_STANDALONE | ||
| #include <boost/integer/common_factor_rt.hpp> | ||
| #endif | ||
| #if (defined(__cpp_lib_gcd_lcm) && (__cpp_lib_gcd_lcm >= 201606L)) && (!defined(BOOST_HAS_INT128) || !defined(__STRICT_ANSI__)) |
There was a problem hiding this comment.
I wonder if we really need the && (!defined(BOOST_HAS_INT128) || !defined(__STRICT_ANSI__)) part? Ah wait... no std::gcd and std::lcm for __int128? I know we have our own gcd/lcm as a fall-back of last resort, but they're not actually very performant (I suspect anyway) compared to the full Boost versions. Or perhaps I'm wrong on that?
In any case we need to get these CI tests fixed up before we can do much of anything... :(
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #676 +/- ##
=========================================
+ Coverage 94.1% 94.1% +0.1%
=========================================
Files 280 280
Lines 29067 29074 +7
=========================================
+ Hits 27340 27347 +7
Misses 1727 1727
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Hmmm... That's a good one. I would be somewhat leary of incurring a performance loss for this. The real changes I implemented involve calculating the I could work out some perf-tests before/after.
Well, it can go green and just did. We can also drop this one any time, it is not a critical change. But I'll invest the hour of play-time for some perf-experiments... Thanks John! |
Well, the performance numbers took a while, but here they are. Care was taken to measure the time of the GCD operations themselves insofar as possible (maybe with extra time only for So it does, in fact, appear that the full Boost versions of GCD in Cc: @jzmaddock and @mborland
|
This part, however, might be a useful change. Cc: @jzmaddock and @mborland |
|
Add the source codes used for performance measurements. It is a zip archive of the source files. |
|
Hi @jzmaddock and @mborland I've never really follwed the Circle CI build that much. For some reason, on trivial changes, the |
|
OK I made a bit of a little mess. I'll need a few tries to clean this up... In progress |
No, but it's started failing on Math as well. I think something outside our control has changed, will try to take a look... |
|
See comment: boostorg/inspect@0ef7948#r156401450 |
|
Merging in acceptance of upstream break in |
In cpp_int/misc.hpp, there is a dependency on
Boost.Integer.But if and only if it is not standalone. Yet the standalone version does not need integer. I was wondering if it is possible to remove this dependency in the non-standalone mode? Thereby the behavior would be the same for
constexpr_gcdin standalone as well as non-standalone mode.Why? I got some pesky warnings coming in from
Boost.Integer.The header has also been refactored to treat having/not-having
std::gcdin a uniform fashion with respect to compiler switches. The compiler switches were not the same in the two places earlier.Cc: @jzmaddock and @mborland