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 support for Floats as the first argument in Time.at. #609

Merged
merged 1 commit into from Apr 16, 2013

Conversation

Projects
None yet
3 participants
@sluukkonen
Copy link
Contributor

sluukkonen commented Mar 27, 2013

While investigating #565, I noticed that JRuby converts a Float argument given to Time.at to a Java long, thus losing some precision.

This commit fixes that, plus keeps track of nanoseconds overflowing to the next millisecond, which is possible when the first argument is not a Fixnum.

@sluukkonen

This comment has been minimized.

Copy link
Contributor Author

sluukkonen commented Mar 27, 2013

Looking at MRI source, it seems to handle this by calling #to_r and rubyspec agrees with that. Maybe we want to do something similar instead?

@sluukkonen

This comment has been minimized.

Copy link
Contributor Author

sluukkonen commented Apr 2, 2013

Looks like the MRI fix for #565 was backported to 1.9.3 https://bugs.ruby-lang.org/issues/8194.

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Apr 11, 2013

Could you squash the two commits into one? Also, if could you check if RubySpec needs a spec for this?

Add support for Floats as the first argument to Time.at.
- Keep track of nanoseconds overflowing to the next millisecond.
@sluukkonen

This comment has been minimized.

Copy link
Contributor Author

sluukkonen commented Apr 15, 2013

I squashed the the commits and rebased against master.

Rubyspec covers this, more or less, but it expects Floats to be converted using to_r. Fixnums, Bignums and Rationals are allowed as is, anything else that responds to to_r but not to_int gets converted to a Rational with to_r. This includes Floats as well. These specs fail with JRuby.

I have a version that does the conversion with to_r as well, but to me, it seems like more like a unnecessary implementation detail of MRI. The documentation explains that either argument maybe one of "Integer, Float, Rational", but it doesn't say anything about to_r being used internally.

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 16, 2013

I'm testing your non-to_r version right now. Unfortunately to_r may be intentional behavior, whether it is documented or not. This is definitely a grey area for the Ruby spec.

I would suggest you modify that patch to be based on this one, and we can merge this in so that the majority of cases that don't use to_r will start working. Then, you should file an issue asking for clarification of whether the to_r behavior is intentional and part of the specification or not.

headius added a commit that referenced this pull request Apr 16, 2013

Merge pull request #609 from sluukkonen/time-at-fixes
Add support for Floats as the first argument in Time.at.

@headius headius merged commit cb556ed into jruby:master Apr 16, 2013

1 check failed

default The Travis build failed
Details
@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 16, 2013

Please open a second PR with the to_r calls if it turns out that ruby-core/matz believe that behavior is specified and required. Thanks so much for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.