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

test: Add negative NaN test cases #475

Merged
merged 1 commit into from
Aug 13, 2020
Merged

test: Add negative NaN test cases #475

merged 1 commit into from
Aug 13, 2020

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Aug 12, 2020

No description provided.

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #475 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #475   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          54       54           
  Lines       16226    16265   +39     
=======================================
+ Hits        16152    16191   +39     
  Misses         74       74           

@gumb0 gumb0 requested review from chfast and axic August 12, 2020 09:20
EXPECT_THAT(execute(*instance, 0, {cnan, anan}), ArithmeticNaN(TypeParam{}));
EXPECT_THAT(execute(*instance, 0, {cnan, -anan}), ArithmeticNaN(TypeParam{}));
EXPECT_THAT(execute(*instance, 0, {-cnan, anan}), ArithmeticNaN(TypeParam{}));
EXPECT_THAT(execute(*instance, 0, {-cnan, -anan}), ArithmeticNaN(TypeParam{}));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we check the sign of the nan on the result side?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is not requires as any sign is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The spec restricts only the payload of the propagated NaN.

Copy link
Member

Choose a reason for hiding this comment

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

The spec restricts only the payload of the propagated NaN.

That means it could be any sign?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

I mean in the above example, it is CPU specific whether -cnan, -anan will result in anan or -anan?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both cnan and -cnan are wasm canonical NaNs. Similarly for arithmetic NaNs. The sign is ignored.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know how CPUs propagate the sign bit, it probably depends on the opcode, too.

Copy link
Member

Choose a reason for hiding this comment

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

I'd add a todo here to investigate this further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a TODO comment.

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Apart from the todo this is fine.

@gumb0 gumb0 merged commit a8040db into master Aug 13, 2020
@gumb0 gumb0 deleted the negative-nan branch August 13, 2020 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants