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

get current time: Time.now, Time.utc_now #4

Merged
merged 1 commit into from
Jan 3, 2017

Conversation

bcardiff
Copy link
Collaborator

@bcardiff bcardiff commented Jan 3, 2017

The following snippets work.

puts Time.now
puts Time.utc_now

simulate TZ using $ TZ="America/New_York" wine ./foo.exe

Note: this does not handle daylight savings.

@bcardiff
Copy link
Collaborator Author

bcardiff commented Jan 3, 2017

This didn't make travis happy :-( hold a bit please.

@bcardiff
Copy link
Collaborator Author

bcardiff commented Jan 3, 2017

Travis is happy again. Ready to go if you think the path taken is good.

I think it would be great if you can enable travis on this fork and enable builds for PR. I hope other will join forces :-)

@lbguilherme
Copy link
Owner

Travis has been enabled! 🎉

I've made some review comments, but I'm not sure how it notifies you (after the review system changed). Did you see them?

@bcardiff
Copy link
Collaborator Author

bcardiff commented Jan 3, 2017

I see no review comments. After writing them yo need to click on the "review changes" button in the top right to send them all.

struct FileTime
low_date_time : DWord
high_date_time : DWord
end
Copy link
Owner

Choose a reason for hiding this comment

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

This could be a single date_time : UInt64 to avoid calculation later

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 way it matches docs. UInt64 would work since it will be guaranteed to be aligned at it is used as out in the method call.

In the docs it is warned the following thing:

Do not cast a pointer to a FILETIME structure to either a ULARGE_INTEGER* or __int64* value because it can cause alignment faults on 64-bit Windows.

If struct FileTime to UInt64 there will be no problem since it is the cast is in the other way, but it is misleading.

What do you prefer?

Copy link
Owner

Choose a reason for hiding this comment

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

It would even be safe to declare it as GetSystemTimeAsFileTime(time : UInt64*). The problem would be if the struct were declared as UInt32, allocated as such and then accessed by a Uint64* if it were on a architecture were accessing misaligned memory caused a crash (This causes problems on ARM, and Window can run on ARM). But it is pretty safe to declare those pairs of high/low DWords as a single UInt64 as long as the struct is allocated by you and not returned from a function.

Are you a lot against doing it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I prefer safe for know, yes. Rust does the same in https://github.com/rust-lang/rust/blob/b1363a73ede57ae595f3a1be2bb75d308ba4f7f6/src/libstd/sys/windows/time.rs

Changing to alias FileTime = UInt64 can be done later I think.
The div and % are much heavier that the bitwise operations.

We can ask for a third vote :-)

Copy link
Owner

Choose a reason for hiding this comment

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

Well... let's do as documented. This can be changed later if proven to be better. Merging.

fun get_time_zone_information = GetTimeZoneInformation(lpTimeZoneInformation : TimeZoneInformation*) : DWord

@[CallConvention("X86_StdCall")]
fun get_system_time_as_file_time = GetSystemTimeAsFileTime(lpSystemTimeAsFileTime : FileTime*)
Copy link
Owner

Choose a reason for hiding this comment

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

(opinated) Better use snake_case for argument names here, even though it contradicts the original name from MSDN docs. I would also give simpler names like tz_info or time. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

{% else %}
# TODO handle daylight
# https://msdn.microsoft.com/en-us/library/windows/desktop/ms724421(v=vs.85).aspx
LibWindows.get_time_zone_information(out tz)
Copy link
Owner

Choose a reason for hiding this comment

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

Docs says it returns TIME_ZONE_ID_INVALID in case of error. Add a check here:

if LibWindows.get_time_zone_information(out tz) == LibWindows::TIME_ZONE_ID_INVALID
  raise WinError.new("GetTimeZoneInformation")
end

The possible return values are:

TIME_ZONE_ID_INVALID = -1
TIME_ZONE_ID_UNKNOWN = 0
TIME_ZONE_ID_STANDARD = 1
TIME_ZONE_ID_DAYLIGHT = 2

@@ -619,6 +627,11 @@ struct Time
ret = LibC.gettimeofday(out timeval, nil)
raise Errno.new("gettimeofday") unless ret == 0
{timeval.tv_sec, timeval.tv_usec.to_i64 * 10}
{% elsif flag?(:windows) %}
ret = LibWindows.get_system_time_as_file_time(out filetime)
Copy link
Owner

Choose a reason for hiding this comment

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

GetSystemTimeAsFileTime returns Void

{% else %}
# TODO handle daylight?
# https://msdn.microsoft.com/en-us/library/windows/desktop/ms724421(v=vs.85).aspx
LibWindows.get_time_zone_information(out tz)
Copy link
Owner

Choose a reason for hiding this comment

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

Check the return value here:

if LibWindows.get_time_zone_information(out tz) == TIME_ZONE_ID_UNKNOWN
  raise WinError.new("GetTimeZoneInformation")
end

The constants are:

internal const int TIME_ZONE_ID_INVALID = -1;
internal const int TIME_ZONE_ID_UNKNOWN = 0;
internal const int TIME_ZONE_ID_STANDARD = 1;
internal const int TIME_ZONE_ID_DAYLIGHT = 2;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unknown should not raise.

TIME_ZONE_ID_UNKNOWN: Daylight saving time is not used in the current time zone, because there are no transition dates or automatic adjustment for daylight saving time is disabled.

Invalid yes.

If the function fails for other reasons, such as an out of memory error, it returns TIME_ZONE_ID_INVALID. To get extended error information, call GetLastError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants