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 proper f{32,64}_add tests #467

Merged
merged 4 commits into from
Aug 11, 2020
Merged

test: Add proper f{32,64}_add tests #467

merged 4 commits into from
Aug 11, 2020

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Aug 10, 2020

It adds/fixes some testing helpers for floating point instructions.

Then it adds wasm spec-driver tests for fadd, i.e. it adds checks for these points from the definition:
image

@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

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

@@           Coverage Diff            @@
##           master     #467    +/-   ##
========================================
  Coverage   99.53%   99.53%            
========================================
  Files          54       54            
  Lines       15821    15926   +105     
========================================
+ Hits        15747    15852   +105     
  Misses         74       74            

Base automatically changed from float-comparison to master August 10, 2020 15:25
@chfast chfast force-pushed the fp_add branch 6 times, most recently from 017c219 to 9e55d85 Compare August 11, 2020 09:30
@chfast chfast force-pushed the fp_add branch 2 times, most recently from eb547f4 to 1a740b6 Compare August 11, 2020 13:17
@@ -121,9 +92,21 @@ template <typename T>
class execute_floating_point_types : public testing::Test
{
protected:
using L = typename FP<T>::Limits;

// The q is any positive floating-point value without zeros, infinities and NaNs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why "q"?

Copy link
Collaborator Author

@chfast chfast Aug 11, 2020

Choose a reason for hiding this comment

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

The meta variable q is used to range over (signless) rational magnitudes, excluding nan or ∞.

https://webassembly.github.io/spec/core/exec/numerics.html#numerics

Copy link
Collaborator

Choose a reason for hiding this comment

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

That includes zeroes, but it's not so important

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call these two maybe positive_special_values and ordered_special_values

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

EXPECT_THAT(exec(-TypeParam{0.0}, Limits::infinity()), Result(Limits::infinity()));
EXPECT_THAT(exec(-TypeParam{0.0}, -Limits::infinity()), Result(-Limits::infinity()));

for (const auto q : this->q_values)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (const auto q : this->q_values)
for (const auto q : q_values)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does not work because of whatever is under GTest macros. You need this-> or execute_floating_point_types::.

class execute_floating_point_types : public testing::Test
{
protected:
using L = typename FP<T>::Limits;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer this to be Limits

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It cannot be because it will conflict of other using Limits in the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't it use this one in the tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because each TYPED_TEST inherits a template execute_floating_point_types<TypeParam> you have to access it as typename execute_floating_point_types<TypeParam>::Limits. Not very short.

@chfast chfast force-pushed the fp_add branch 3 times, most recently from 50f007b to 403e427 Compare August 11, 2020 15:29
@chfast chfast merged commit 6ba5313 into master Aug 11, 2020
@chfast chfast deleted the fp_add branch August 11, 2020 21:50
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