Skip to content

Convert round calls to math.Round#777

Open
icholy wants to merge 3 commits into
elliotchance:masterfrom
icholy:master
Open

Convert round calls to math.Round#777
icholy wants to merge 3 commits into
elliotchance:masterfrom
icholy:master

Conversation

@icholy

@icholy icholy commented Jul 10, 2018

Copy link
Copy Markdown

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

@elliotchance elliotchance left a comment

Copy link
Copy Markdown
Owner

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

@icholy icholy left a comment

Copy link
Copy Markdown
Author

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.

@icholy icholy left a comment

Copy link
Copy Markdown
Author

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.

@elliotchance elliotchance left a comment

Copy link
Copy Markdown
Owner

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.

@icholy icholy left a comment

Copy link
Copy Markdown
Author

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.

@elliotchance elliotchance left a comment

Copy link
Copy Markdown
Owner

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