-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix exp function to actually work #43
base: master
Are you sure you want to change the base?
Conversation
aa10eb8
to
77719cd
Compare
It was calling into decNumberExp with the wrong signature, so it didn't work at all. I also fixed the decNumber tests for exp so that they'll actually run if point at the exp.decTest file, and I added a super basic test for ln and exp. It seems a bit too easy to not run some of the decNumber test by accident right now; maybe it would be good to expect every test file to have some non-igored tests, and then just delete the test files for actually unimplemented functionality?
77719cd
to
d2efc74
Compare
@@ -540,6 +540,7 @@ expx1200 exp 1 -> 2.718281828459045235360287471352662 Inexact Rounded | |||
Precision: 34 | |||
maxExponent: 6144 | |||
minExponent: -6143 | |||
clamp: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? This test should not change at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of these lines in the run-test script:
let d128_env = Environment {
precision: Some(34),
rounding: Some(Rounding::HalfEven),
max_exponent: Some(6144),
min_exponent: Some(-6143),
extended: true,
clamp: true,
};
if *env != d128_env {
return TestResult::Ignored(test);
}
Without that change, the tests were silently skipped (and none of the other matched either).
Maybe I should have gone in the other direction and defined a different Environment just for these tests? I assume that the check is there to make sure that it only runs applicable tests, but since the exp tests pass with the different clamping mode, I assume that all of those test cases are still valid with the different clamping mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to change run_test()
to run ops with the specified environment which will make this change unnecessary and increase test coverage?
It was calling into decNumberExp with the wrong signature, so it didn't work at all.
I also fixed the decNumber tests for exp so that they'll actually run if point at the exp.decTest file, and I added a super basic test for ln and exp.
It seems a bit too easy to not run some of the decNumber test by accident right now; maybe it would be good to expect every test file to have some non-igored tests, and then just delete the test files for actually unimplemented functionality?