Refactor time platform specific implementation #4502

bcardiff · 2017-06-03T04:43:37Z

Time::Span ticks constants where moved to avoid circular dependency.

ysbaddaden · 2017-06-03T08:13:42Z

Maybe ticks don't have to be moved? Crystal::System::Time.compute_offset could return seconds (same as it's argument) instead of ticks, then have Time.compute_offset transform to ticks. This way, ticks would remain a Time implementation details, unrelated to the system calls that return seconds (and fractions of a second).

RX14 · 2017-06-03T09:05:49Z

My main concern is that taken on their own (as they should be), compute_offset and compute_second_and_tenth_microsecond are weird functions. They make perfect sense in the context of Time but when taken out they seem very arbitrary.

I agree with @ysbaddaden that compute_offset should return seconds, and probably be renamed to something more descriptive like compute_utc_offset or compute_local_offset. Additionally, I think compute_second_and_tenth_microsecond should be called compute_utc and just return ticks. The unix epoch is a platform specific thing (windows uses 1 Jan 1601 as an epoch) so i'd rather the method do more.

ysbaddaden · 2017-06-03T09:37:56Z

I prefer an explicit{seconds, fraction) tuple over ticks. I agree the epoch should be applied since it's system related, so seconds means seconds from 0 instead is seconds since X. Unless we introduce a system specific EPOCH constant maybe? Thought I'm not sure it would useful.

RX14 · 2017-06-03T10:00:54Z

@ysbaddaden the tuple is so that we don't need to do conversions back and forth from ticks, correct?

RX14 · 2017-06-03T10:39:21Z

src/crystal/system/time.cr

@@ -0,0 +1,22 @@
+# :nodoc:


We don't want to :nodoc: Crystal as we loose documenting macros and Crystal::VERSION. The :nodoc: should be placed on Crystal::System only.

My bad. I did that on the random PR!

fixed (in master as well)

RX14 · 2017-06-03T10:44:21Z

src/crystal/system/unix/time.cr

+  end
+
+  def self.compute_second_and_tenth_microsecond
+    {% if flag?(:darwin) %}


Should this be separated out into a file just for darwin? How about requiring this (unix) file and a darwin file which overrides compute_second_and_tenth_microsecond or would that be too messy?

It happens once and it's 3 lines. It seems more complicated to extract it... so I guess it's acceptable?

I'd move it to a separate file. I think inside a given System implementation there shouldn't be any if flag?, otherwise it kind of missing the point of extracting it into a different place.

Splitting darwin and unix makes this harder to understand I think. One function is shared. The other don't. I tried splitting them but I was not happy with the result. Now the responsibilities of the System::Time is more clear and the windows implementation will be in a different file.

I believe macOS Sierra added support for clock_gettime, so maybe the condition will go away, someday.

bcardiff · 2017-06-05T03:53:20Z

Functions renamed. Now the {seconds, fraction} is from absolute 0. Which kind of match the ::Time representation a bit more.

The only platform specific in ::Time are now a constructor that references LibC::Timespec which is used in File::Stat.

Rename methods

bcardiff · 2017-06-17T23:19:53Z

Since the seconds in Crystal::System::Time.compute_utc_offset is now absolute from 0 and always an Int64, in the 32bits platform there was an issue in when calling LibC.localtime_r(pointerof(seconds), out tm) . That make me notice an issue that UnixEpochInSeconds was not been subtracted from the argument. Now that is done and a seconds_from_epoch = LibC::TimeT.new(seconds - UnixEpochInSeconds) ensures the right time is used for LibC.localtime_r(pointerof(seconds_from_epoch), out tm)

bcardiff · 2017-06-20T14:15:57Z

@RX14 , @ysbaddaden I would appreciate another pair of eyes before merging this. I fix an issue discovered by 32 bits, hopefully the only thing left is #4502 (comment) .

ysbaddaden · 2017-06-20T15:27:28Z

I think the API will eventually change, but for the time being this is fine. Maybe replace if flag?(:darwin) to something more generic, that would play well if C bindings are automatically generated, someday (and would pick clock_gettime on macOS >= Sierra but gettimeofday for older releases):

if LibC.methods.includes?("clock_gettime".id)
  # clock_gettime
else
  # fallback to gettimeofday
end

BTW: what may change is that we need a broken down time representation to determine the offset, using tm + mktime on POSIX, or SYSTEMTIME + DYNAMIC_TIME_ZONE_INFORMATION on Windows, or later from IANA tables directly. For example In my add-offset-to-time refactor, I currently have this:
https://gist.github.com/ysbaddaden/75ad2b4a4f75be92757b4d253cfa885b

RX14 · 2017-06-20T15:45:36Z

src/crystal/system/time.cr

+    # :nodoc:
+    module Time
+      # Returns the number of seconds that you must add to UTC to get local time.
+      # *seconds* are absolutes.


What do you mean by *seconds* are absolutes?

It means seconds elapsed from 0, not from unix epoch as it was before.

When you say "from 0" it's not at all clear you're referring to the Gregorian epoch. I'd say something like *seconds* are measured from the Gregorian epoch (01 January 0001 00:00:00)

RX14 · 2017-06-20T15:45:43Z

src/crystal/system/time.cr

+      # *seconds* are absolutes.
+      # def self.compute_utc_offset(seconds)
+
+      # Returns the current utc time meassured in absolute `{seconds, tenth_microsecond}`


Add type some annotations to Crystal::Sytem::Time methods

bcardiff added this to Issues in Windows Jun 3, 2017

RX14 requested changes Jun 3, 2017

View reviewed changes

Brian J. Cardiff added 3 commits June 17, 2017 20:02

Refator time platform specific implementation

215bde4

Remove Ticks constants from crystal/system

7a5b374

Rename methods

Make System::Time return absolute seconds instead of since epoch

c6db558

bcardiff force-pushed the time branch from 112292a to 4f3a62e Compare June 17, 2017 23:04

bcardiff changed the title ~~Refator time platform specific implementation~~ Refactor time platform specific implementation Jun 17, 2017

Ensure localtime_r is called with seconds from unix epoch

950b53d

bcardiff force-pushed the time branch from 4f3a62e to 950b53d Compare June 17, 2017 23:15

RX14 reviewed Jun 20, 2017

View reviewed changes

Brian J. Cardiff added 3 commits June 21, 2017 00:29

Remove :darwin flag check in favor of a more generic condition

19adcb9

Update docs

e0b9fa2

Fix time in 32 bits when gettimeofday is available

e3b5b25

Add type some annotations to Crystal::Sytem::Time methods

bcardiff merged commit e92d280 into crystal-lang:master Jun 25, 2017

bcardiff added kind:refactor topic:stdlib labels Jun 25, 2017

bcardiff added this to the Next milestone Jun 25, 2017

Sija referenced this pull request Jun 27, 2017

Crystal 0.23.0 Changelog

3c3d3e2

straight-shoota moved this from Done to Stdlib in Windows Jan 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor time platform specific implementation #4502

Refactor time platform specific implementation #4502

bcardiff commented Jun 3, 2017

ysbaddaden commented Jun 3, 2017

RX14 commented Jun 3, 2017 •

edited

ysbaddaden commented Jun 3, 2017

RX14 commented Jun 3, 2017 •

edited

RX14 Jun 3, 2017

ysbaddaden Jun 3, 2017

bcardiff Jun 5, 2017

RX14 Jun 3, 2017 •

edited

ysbaddaden Jun 3, 2017

asterite Jun 3, 2017

bcardiff Jun 5, 2017

ysbaddaden Jun 5, 2017

bcardiff commented Jun 5, 2017

bcardiff commented Jun 17, 2017

bcardiff commented Jun 20, 2017

ysbaddaden commented Jun 20, 2017

RX14 Jun 20, 2017

bcardiff Jun 20, 2017

RX14 Jun 20, 2017

RX14 Jun 20, 2017 •

edited

Refactor time platform specific implementation #4502

Refactor time platform specific implementation #4502

Conversation

bcardiff commented Jun 3, 2017

ysbaddaden commented Jun 3, 2017

RX14 commented Jun 3, 2017 • edited

ysbaddaden commented Jun 3, 2017

RX14 commented Jun 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Jun 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardiff commented Jun 5, 2017

bcardiff commented Jun 17, 2017

bcardiff commented Jun 20, 2017

ysbaddaden commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Jun 20, 2017 • edited

Choose a reason for hiding this comment

RX14 commented Jun 3, 2017 •

edited

RX14 commented Jun 3, 2017 •

edited

RX14 Jun 3, 2017 •

edited

RX14 Jun 20, 2017 •

edited