Skip to content

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 added the shopify Pull requests from Shopify label Feb 2, 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 Pull requests from Shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants