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

Time#floor and ceil #2201

Merged
merged 4 commits into from
Jan 4, 2021
Merged

Time#floor and ceil #2201

merged 4 commits into from
Jan 4, 2021

Conversation

chrisseaton
Copy link
Collaborator

@chrisseaton chrisseaton commented Dec 23, 2020

On behalf of Maple, but not pinging her as she's on leave.

Shopify#1

@@ -228,6 +228,24 @@ def round(places = 0)
time + (rounded - original)
end

def floor(places = 0)
original = to_i + subsec.to_r
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't Time#subsec already return a Rational and Rational#to_r is a no-op?
I see, there is a special case when subsec is zero:

> Time.new(0).subsec
=> 0

floored = original.floor(places)

time = Time.allocate
time.send(:initialize_copy, self)
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply time = dup here?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, because the spec says so, alright:

    it "returns an instance of Time, even if #floor is called on a subclass" do
      subclass = Class.new(Time)
      instance = subclass.at(0)
      instance.class.should equal subclass
      instance.floor.should be_an_instance_of(Time)
    end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a common pattern isn't it? Maybe we should have some kind of Primitive.dup_as_class(x, T)

Copy link
Member

Choose a reason for hiding this comment

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

Calling initialize_copy explicitly seems to be only used for Time so far.
Other classes probably build the new instance directly and pass all required state.
The issue with Time is there isn't a nice constructor to use, they are all very complicated.

Maybe we could have a Truffle::TimeOperations helper, but it seems a bit overkill.

@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Jan 4, 2021
@eregon eregon self-assigned this Jan 4, 2021
@graalvmbot graalvmbot merged commit 9d259b9 into oracle:master Jan 4, 2021
@chrisseaton chrisseaton deleted the time-floor-ceil branch August 30, 2021 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants