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

try harder to avoid long overflow in Time#+ (fixes #1779) #4646

Merged
merged 4 commits into from Jun 5, 2017

Conversation

Projects
None yet
3 participants
@MSNexploder
Contributor

MSNexploder commented Jun 5, 2017

No description provided.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jun 5, 2017

Member

... very nice Stefan, could we have a test or spec please (as this is easy to break at some point) ?

Member

kares commented Jun 5, 2017

... very nice Stefan, could we have a test or spec please (as this is easy to break at some point) ?

@MSNexploder

This comment has been minimized.

Show comment
Hide comment
@MSNexploder

MSNexploder Jun 5, 2017

Contributor

Added a small test (and reenabled some existing time related regression tests which pass) but i'm not sure if this is the best way to test.

Contributor

MSNexploder commented Jun 5, 2017

Added a small test (and reenabled some existing time related regression tests which pass) but i'm not sure if this is the best way to test.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jun 5, 2017

Member

@MSNexploder thanks, its not bad (you can leave it in) but does not actually test for the reported issue,
a new test which would test smt like :

t = Time.local(2000, 1, 1) + (300 * 366 * 24 * 60 * 60)
assert_equal 2300, t.year

... would probably do, what do you think?

Member

kares commented Jun 5, 2017

@MSNexploder thanks, its not bad (you can leave it in) but does not actually test for the reported issue,
a new test which would test smt like :

t = Time.local(2000, 1, 1) + (300 * 366 * 24 * 60 * 60)
assert_equal 2300, t.year

... would probably do, what do you think?

@kares kares added this to the JRuby 9.1.11.0 milestone Jun 5, 2017

@MSNexploder

This comment has been minimized.

Show comment
Hide comment
@MSNexploder

MSNexploder Jun 5, 2017

Contributor

@kares That was too easy 😎 Was focusing way to much on the underlying problem.

Contributor

MSNexploder commented Jun 5, 2017

@kares That was too easy 😎 Was focusing way to much on the underlying problem.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jun 5, 2017

Member

cool thanks a lot ... @headius could you double-check this is good to go for @MSNexploder, please?

Member

kares commented Jun 5, 2017

cool thanks a lot ... @headius could you double-check this is good to go for @MSNexploder, please?

@headius

headius approved these changes Jun 5, 2017

@headius headius merged commit dad5ac5 into jruby:master Jun 5, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 5, 2017

Member

Looks good, thank you!

Member

headius commented Jun 5, 2017

Looks good, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment