Refactor time platform specific implementation

[bcardiff]

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

[ysbaddaden]

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]

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]

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]

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

[RX14]

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

[ysbaddaden]

My bad. I did that on the random PR!

[bcardiff]

fixed (in master as well)

[RX14]

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?

[ysbaddaden]

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

[asterite]

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.

[bcardiff]

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.

[ysbaddaden]

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

[bcardiff]

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.

[bcardiff]

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]

@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]

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]

What do you mean by *seconds* are absolutes?

[bcardiff]

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

[RX14]

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]

ditto