[rational] Can the community maintenance team and/or rational users please review some pull requests?
I have a bunch of PR's against Rational, which of course is basically unmaintained. I'd be grateful if a second pair of eyes (at least) could take a look before I start agitating for someone to merge them ;) Each of the four, builds on the previous one(s). Three are basically trivial: https://github.com/boostorg/rational/pull/1/files https://github.com/boostorg/rational/pull/2/files https://github.com/boostorg/rational/pull/3/files The final one is much more complex, and disables "accidental" conversion from float to rational (which otherwise proceeds via truncation to integer first). As you can see from the commit history, I've had quite a few attempts at getting the final one correct. I believe it is now, doesn't break anything in Boost except, as noted in the discussion, Boost.Geometry, which relies on an unsafe float to rational conversion - Geometry's authors are aware of the issue here: https://github.com/boostorg/geometry/commit/c992eb61e8ef9e90fdae403ac6ff1eea.... So.... I'm interested in both eyeball-look-throughs from seasoned Boosters, but also feedback from Rational's users, and verification that it doesn't break end user code - other than unsafe conversions of course. If you just want the updated header to test, you can grab it from here: https://raw.githubusercontent.com/jzmaddock/rational/no-float-convert/includ... Many thanks! John.
Forgot to say, this is also issue: https://svn.boost.org/trac/boost/ticket/10244 (although that trac issue doesn't actual deal with all the problems). There are also some other open issues that look like they're low hanging fruit, but one thing at a time... John. On 30/03/2015 12:44, John Maddock wrote:
I have a bunch of PR's against Rational, which of course is basically unmaintained. I'd be grateful if a second pair of eyes (at least) could take a look before I start agitating for someone to merge them ;)
Each of the four, builds on the previous one(s).
Three are basically trivial:
https://github.com/boostorg/rational/pull/1/files https://github.com/boostorg/rational/pull/2/files https://github.com/boostorg/rational/pull/3/files
The final one is much more complex, and disables "accidental" conversion from float to rational (which otherwise proceeds via truncation to integer first).
As you can see from the commit history, I've had quite a few attempts at getting the final one correct. I believe it is now, doesn't break anything in Boost except, as noted in the discussion, Boost.Geometry, which relies on an unsafe float to rational conversion - Geometry's authors are aware of the issue here: https://github.com/boostorg/geometry/commit/c992eb61e8ef9e90fdae403ac6ff1eea....
So.... I'm interested in both eyeball-look-throughs from seasoned Boosters, but also feedback from Rational's users, and verification that it doesn't break end user code - other than unsafe conversions of course. If you just want the updated header to test, you can grab it from here: https://raw.githubusercontent.com/jzmaddock/rational/no-float-convert/includ...
Many thanks! John.
On Mon, 30 Mar 2015, John Maddock wrote:
I have a bunch of PR's against Rational, which of course is basically unmaintained. I'd be grateful if a second pair of eyes (at least) could take a look before I start agitating for someone to merge them ;)
Each of the four, builds on the previous one(s).
Three are basically trivial:
https://github.com/boostorg/rational/pull/1/files https://github.com/boostorg/rational/pull/2/files https://github.com/boostorg/rational/pull/3/files
The final one is much more complex, and disables "accidental" conversion from float to rational (which otherwise proceeds via truncation to integer first).
As you can see from the commit history, I've had quite a few attempts at getting the final one correct. I believe it is now, doesn't break anything in Boost except, as noted in the discussion, Boost.Geometry, which relies on an unsafe float to rational conversion - Geometry's authors are aware of the issue here: https://github.com/boostorg/geometry/commit/c992eb61e8ef9e90fdae403ac6ff1eea....
So.... I'm interested in both eyeball-look-throughs from seasoned Boosters, but also feedback from Rational's users, and verification that it doesn't break end user code - other than unsafe conversions of course. If you just want the updated header to test, you can grab it from here: https://raw.githubusercontent.com/jzmaddock/rational/no-float-convert/includ...
If I understand correctly, cpp_rational is constructible from double but rational isn't? It is documented, so it's ok, but it may still confuse some users. (I have never used either, but I use some alternatives, and I am planning to use cpp_rational soon, and construction from double is necessary for my use (hence ticket 10082, thanks again for fixing that so quickly)) -- Marc Glisse
If I understand correctly, cpp_rational is constructible from double but rational isn't? It is documented, so it's ok, but it may still confuse some users.
Correct. Conversion works in the multiprecision case because someone asked for it, and the integers are large enough that we shouldn't have to worry about overflow etc. In the non-multiprecision case, there are plenty of doubles which can't be converted to a rational of (built in) integers. And of course (possibly 128 bit) long doubles are even more of an issue. John.
2015-03-30 19:16 GMT+02:00 Marc Glisse
If I understand correctly, cpp_rational is constructible from double but rational isn't?
That is incorrect rational is (inadvertently) constructible from double.
Check it out (with 1.57):
#include
That is incorrect rational is (inadvertently) constructible from double. Check it out (with 1.57):
#include
int main() { boost::rational<int> r = 2.5; assert (boost::rational<int>(2) == r); }
With an unfortunate results. This is a serious bug.
Correct: that is the whole point of this proposed change - to fix that bug. John.
On Wed, Apr 1, 2015 at 5:40 AM, John Maddock
That is incorrect rational is (inadvertently) constructible from double.
Check it out (with 1.57):
#include
int main() { boost::rational<int> r = 2.5; assert (boost::rational<int>(2) == r); }
With an unfortunate results. This is a serious bug.
Correct: that is the whole point of this proposed change - to fix that bug.
I have achieved write permissions on Boost.Rational. (about two hours ago). I merged requests #1, #2, #3. -- Marshall
I have achieved write permissions on Boost.Rational. (about two hours ago).
I merged requests #1, #2, #3. Many thanks Marshall.
Where do we stand with the - admittedly more difficult - #4? I assume we all agree that accidental conversion from float is a serious loophole in the library, the question I guess is whether this is the right fix? Cheers, John.
AMDG On 03/30/2015 05:44 AM, John Maddock wrote:
This is at least potentially problematic because one side of ?: has type NewType, but the other has type int. In Christ, Steven Watanabe
participants (5)
-
Andrzej Krzemienski -
John Maddock -
Marc Glisse -
Marshall Clow -
Steven Watanabe