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 float type and test cases for float and double type. #187

Merged
merged 12 commits into from
Nov 25, 2017
Merged

Add float type and test cases for float and double type. #187

merged 12 commits into from
Nov 25, 2017

Conversation

usefulhyun
Copy link
Contributor

@usefulhyun usefulhyun commented Nov 15, 2017

This patch includes the implementation of float type
and extensive test cases for float and double type.

  • implementation of float type and its test interface.
  • test cases for float and double type.

ISSUE=#182

Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

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

Looks good overall.
I left some comments.


expect(test_interface.floatMethod(0.0)).toBe(0.0);

var base = 1 / 2
Copy link
Member

Choose a reason for hiding this comment

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

Please use const instead of var.
Also, you missed semi-colon ;.

// the same, so we can not test the value out of the range.

// NaN == NaN returns false, thus we must use isNaN function
expect(isNaN(test_interface.doubleMethod(NaN))).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, float and double SHOULD NOT take these special values such as NaN, Infinity and -Infinity.
They are only allowed in unrestricted float and unrestricted double.
Please see: https://heycam.github.io/webidl/#idl-unrestricted-float

Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

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

@usefulhyun Ah, I didn't know that you already updated this patch.
Sorry for delayed review.

// unrestricted.
const base = 1 / 2;
const precision = 52;
for (var test_case = 0; test_case < 100; test_case++) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use let instead of var. The var is function scope and let is block scope.
So, we prefer to use let to avoid unexpected behavior.

@romandev romandev merged commit d47d88f into lunchclass:master Nov 25, 2017
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.

2 participants