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

Implement Time.utc and Time.local #2223

Open
b-n opened this issue Oct 27, 2022 · 0 comments
Open

Implement Time.utc and Time.local #2223

b-n opened this issue Oct 27, 2022 · 0 comments
Assignees
Labels
A-ruby-core Area: Ruby Core types. B-mruby Backend: Implementation of artichoke-core using mruby. C-enhancement Category: New feature or request. E-medium Call for participation: Experience needed to fix: Medium / intermediate.

Comments

@b-n
Copy link
Member

b-n commented Oct 27, 2022

Normally I'd just create a PR for this, however it looks like a not insignificant amount of work in order to get there due to the way the way these functions in the title work.

Docs and context

Ruby Docs Example: https://ruby-doc.org/core-3.1.2/Time.html#method-c-utc

Note: The function can take 1..8 args or 10 args. The errors in MRI for 9 args implies that the 10 arg variant does not exist:

[3.1.2] > Time.utc(1,2,3,4,5,6,7,8,9)
(irb):33:in `utc': wrong number of arguments (given 9, expected 1..8) (ArgumentError)
	from (irb):33:in `<main>'                                
	from /home/ben/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
	from /home/ben/.rbenv/versions/3.1.2/bin/irb:25:in `load'
	from /home/ben/.rbenv/versions/3.1.2/bin/irb:25:in `<main>'

The reason for the 10 arg support is so the following function works:

[3.1.2] > Time.utc(*Time.now.to_a)   #splat the array of 10 values into args
=> 2022-10-27 10:48:57 UTC

⚠️ The big issue here is that the order of the args in the 1..8 version is different than in the 10 version variant. Specifically this:

utc(year, month=1, day=1, hour=0, min=0, sec_i=0, usec=0) → new_time
utc(sec_i, min, hour, day, month, year, dummy, dummy, dummy, dummy) → new_time 

How MRI/Mruby handle this

MRI solves this as follows: https://github.com/artichoke/artichoke/blob/trunk/artichoke-backend/vendor/ruby/time.c#L2861-L2870 (just re-order the values if we have 10 args). Called from here: https://github.com/artichoke/artichoke/blob/trunk/artichoke-backend/vendor/ruby/time.c#L3362-L3369

MRuby just ignores the 10 var variant: https://github.com/artichoke/artichoke/blob/trunk/artichoke-backend/vendor/mruby/mrbgems/mruby-time/src/time.c#L492-L503

Solution design

I haven't yet seen places in Artichoke where argc/argv are being passed into the trampoline - and by and large mrb_get_args is relied upon for argument checking (and this is still from the mruby source). I think this might be an exception if we're wanting to get to 3.1.2 support, Thus, I think we need a TimeArg - similar to how MRI does it.

@b-n b-n added C-enhancement Category: New feature or request. A-ruby-core Area: Ruby Core types. E-medium Call for participation: Experience needed to fix: Medium / intermediate. B-mruby Backend: Implementation of artichoke-core using mruby. labels Oct 27, 2022
@b-n b-n self-assigned this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ruby-core Area: Ruby Core types. B-mruby Backend: Implementation of artichoke-core using mruby. C-enhancement Category: New feature or request. E-medium Call for participation: Experience needed to fix: Medium / intermediate.
Development

No branches or pull requests

1 participant