Fix negative denominator in boost::rational during exponentiation.
#5348
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5348 +/- ##
===========================================
+ Coverage 88% 88.01% +<.01%
===========================================
Files 322 322
Lines 32489 32494 +5
Branches 3865 3866 +1
===========================================
+ Hits 28593 28599 +6
+ Misses 2592 2591 -1
Partials 1304 1304
|
libsolidity/ast/Types.cpp
Outdated
| @@ -1145,9 +1145,14 @@ TypePointer RationalNumberType::binaryOperatorResult(Token _operator, TypePointe | |||
|
|
|||
| if (exp >= 0) | |||
| value = rational(numerator, denominator); | |||
| else | |||
| else { | |||
There was a problem hiding this comment.
Too long that I haven't written solidity code :-D.
libsolidity/ast/Types.cpp
Outdated
| // invert | ||
| value = rational(denominator, numerator); | ||
| // due to a bug in certain versions of boost the denominator cannot be negative |
There was a problem hiding this comment.
This is rather confusing because you swap numerator and denominator. Would it be better to create a function that is used to construct the rational which then adjusts the signs accordingly?
There was a problem hiding this comment.
Yes, that's probably clearer, I moved it to a makeRational function.
ekpyron
force-pushed
the
boostRationalNegativeDenominatorFix
branch
from
1530632 to
5c63575
Compare
ekpyron
force-pushed
the
boostRationalNegativeDenominatorFix
branch
from
5c63575 to
e036133
Compare
| @@ -51,6 +51,15 @@ using FunctionTypePointer = std::shared_ptr<FunctionType const>; | |||
| using TypePointers = std::vector<TypePointer>; | |||
| using rational = boost::rational<dev::bigint>; | |||
|
|
|||
| inline rational makeRational(bigint const& _numerator, bigint const& _denominator) | |||
There was a problem hiding this comment.
I think this would be better suited in an anonymous namespace inside the cpp file or in a more central place inside libdevcore.
There was a problem hiding this comment.
Actually, I don't think so - in this header we introduce the rational alias and - if we want to stay compatible with the buggy boost version - we will always want to use makeRational, when we create a rational from a numerator and denominator, so I think it makes a lot of sense to have it right here below the using rational = ......
There was a problem hiding this comment.
Or if we want to move it, I'd at least move the using rational = ... part as well.
There was a problem hiding this comment.
Cleanly moving using rational = ... to libdevcore would require changing it's namespace, though - which we could do, but which may have side-effects. So actually I think right here is not too bad of a place for this for now :-).
There was a problem hiding this comment.
I think Types.h would need some cleanup at some point anyway....
Actual fix for the problem in #5347.
During exponentiation negative denominators may occur, resulting in boost 1.68 throwing an exception.