Skip to content
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

Add expm1 test case to cover the k==1024 case #4508

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

lygstate
Copy link
Contributor

@lygstate lygstate commented Jan 18, 2021

Refer to

y = y * 2.0 * 0x1p1023;

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]

@akosthekiss
Copy link
Member

What is 0x1p1023 and why does it need extra coverage? Furthermore, why is the negated value not tested?

@lygstate
Copy link
Contributor Author

What is 0x1p1023 and why does it need extra coverage? Furthermore, why is the negated value not tested?

Refer to

y = y * 2.0 * 0x1p1023;

Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the change requested in the code, please, update commit title and message as well.

Current commit title is misleading, as it is hard to cover a literal/number, unless it is the input value to the function. But it is not the case here. The test case aims at covering the k==1024 case. So, the title should read "Add expm1 test case to cover the k==1024 case".

As for the commit message, don't include the discussion in there. Use comments to answer questions. The commit message should describe what has been done, and why. But rarely something like "refer to ..."

@@ -404,6 +404,7 @@ main (int argc, char **args)
GEN_DBL_TEST (expm1 (INFINITY));
GEN_DBL_TEST (expm1 (-INFINITY));
GEN_DBL_TEST (expm1 (NAN));
GEN_DBL_TEST (expm1 (7.095e+02));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this one line down, between 708 and 710 (and re-generate the .inc.h)

@akosthekiss akosthekiss added jerry-libm Related to the jerry-libm library test Related to testing labels Jan 18, 2021
@lygstate lygstate changed the title Add one expm1 test case to cover 0x1p1023 Add expm1 test case to cover the k==1024 case Jan 18, 2021
@akosthekiss
Copy link
Member

BTW, kind of answer-to-self: 0x1p1023 is a hexadecimal floating point literal with the value of 0x1 * 2**1023. I haven't used them, like ever.

-7.095e+02 won't trigger the  k==1024 case, so we didn't add it into the test list

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]
Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rerobika rerobika merged commit d97540e into jerryscript-project:master Jan 18, 2021
@lygstate lygstate deleted the exmp1_test branch January 18, 2021 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jerry-libm Related to the jerry-libm library test Related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants