Remove uses of boost::rational with negative denominator in tests. #5347
Conversation
|
Related issue in boost: boostorg/rational#27 This is in fact a bug in boost, but I still think it would be better if the 0.5.0 tests wouldn't fail against the current release of boost... Alternatively we could use boost version |
Codecov Report
@@ Coverage Diff @@
## develop #5347 +/- ##
==========================================
- Coverage 88% 88% -0.01%
==========================================
Files 322 322
Lines 32491 32489 -2
Branches 3865 3865
==========================================
- Hits 28595 28593 -2
Misses 2592 2592
Partials 1304 1304
|
Changelog.md
Outdated
| @@ -107,6 +107,7 @@ Compiler Features: | |||
| Bugfixes: | |||
| * Build System: Support versions of CVC4 linked against CLN instead of GMP. In case of compilation issues due to the experimental SMT solver support, the solvers can be disabled when configuring the project with CMake using ``-DUSE_CVC4=OFF`` or ``-DUSE_Z3=OFF``. | |||
| * Tests: Fix chain parameters to make ipc tests work with newer versions of cpp-ethereum. | |||
| * Tests: Do not use negative denominators for ``boost::rational`` in the test. This breaks for ``boost >= 1.68`` and negative denominators never actually occur. | |||
There was a problem hiding this comment.
Actually I think this clutters the changelog unnecessarily.
There was a problem hiding this comment.
Yeah ok - I actually considered not adding an entry, but then added one regardless... I'll remove it.
…s with boost 1.68.
ekpyron
force-pushed
the
boostRationalNegativeDenominator
branch
from
33a715e
to
36903d7
Compare
|
Removed the Changelog entry. |
Did you add an assert for that? |
|
Actually, I think I missed one place where the denominator might actually be negative during exponentiation - I'll verify and fix it (and add a test). It's the only place we use the constructor with a denominator at all, though, and since we can't add an assertion to boost I think with that upcoming change it'll be fine. |
|
Yes, in fact |
boost 1.68.0throws an exception when supplying negative denominators to the constructor ofboost::rational.I'm pretty confident that this is never done in the actual code (I had a look through all occurrences of
rational), but only in the test case adjusted in this PR - if that's true, we can just remove these test cases, but maybe someone should recheck.