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

Convert round calls to math.Round #777

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

icholy
Copy link

@icholy icholy commented Jul 10, 2018

I'm having issues with the INFINITY tests.

   		# round
    		308 ok - isnan(round(NAN))
    		309 not ok - tests/math.c:405: round(INFINITY) == INFINITY # got inf
    		310 not ok - tests/math.c:406: round(-INFINITY) == -INFINITY # got -inf
    		311 ok - round(0) == 0
    		312 ok - round(1) == 1
    		313 ok - round(0.5) == 1
    		314 ok - round(0.4) == 0
    		315 ok - round(1.23e300) == 1.23e300
    		316 ok - round(M_PI) == 3

This change is Reviewable

Copy link
Owner

@elliotchance elliotchance left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @icholy)


tests/math.c, line 406 at r1 (raw file):

  is_nan(round(NAN));
  is_eq(round(INFINITY), INFINITY);
  is_eq(round(-INFINITY), -INFINITY);

You cannot use equality tests with infinities, you can us this instead:

is_inf(round(INFINITY), 1);
is_inf(round(-INFINITY), -1);

Docs here: https://github.com/elliotchance/c2go/blob/master/tests/tests.h#L205-L224

Copy link
Author

@icholy icholy left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @icholy)


tests/math.c, line 406 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

You cannot use equality tests with infinities, you can us this instead:

is_inf(round(INFINITY), 1);
is_inf(round(-INFINITY), -1);

Docs here: https://github.com/elliotchance/c2go/blob/master/tests/tests.h#L205-L224

Derp, that makes perfect sense.

Copy link
Author

@icholy icholy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @elliotchance)


tests/math.c, line 406 at r1 (raw file):

Previously, icholy (Ilia Choly) wrote…

Derp, that makes perfect sense.

Done.

Copy link
Owner

@elliotchance elliotchance left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @icholy)


tests/math.c, line 14 at r1 (raw file):

int main()
{
  plan(359);

You will need to update this to 372. This is an extra safety net to make sure all the tests do actually run.

Copy link
Author

@icholy icholy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @elliotchance)


tests/math.c, line 14 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

You will need to update this to 372. This is an extra safety net to make sure all the tests do actually run.

Done.

Copy link
Owner

@elliotchance elliotchance left a comment

Choose a reason for hiding this comment

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

The build is still failing, do the tests pass locally on your machine?

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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