Bug 83156 – [WIN] Simplify implementation of currentTime()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Patrick R. Gansterer]

[WIN] Simplify implementation of currentTime()

[Patrick R. Gansterer]

Created attachment 135583 [details]
Patch

[Patrick R. Gansterer]

Created attachment 136155 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 136155 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136155&action=review

> Source/WTF/wtf/CurrentTime.cpp:81
> +    union {
> +        FILETIME fileTime;
> +        ULONGLONG quadPart;
> +    } u;

Is a union safe? MSDN says, "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."

[Patrick R. Gansterer]

Created attachment 136262 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 136262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136262&action=review

> Source/WTF/wtf/CurrentTime.cpp:86
> +    ULONGLONG dateTime = (static_cast<ULONGLONG>(fileTime.dwHighDateTime) << 32) | fileTime.dwLowDateTime;

I think it would be nicer to copy fileTime into a ULARGE_INTEGER (without using a union) and then use its QuadPart member. That's what MSDN recommends. Sorry for not being more explicit before.

[Patrick R. Gansterer]

Created attachment 144916 [details]
Patch

[Brent Fulgham]

Comment on attachment 144916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144916&action=review

I think this change looks great.  Please consider retaining the comments I mention in the detailed notes when you commit the change.

> Source/WTF/wtf/CurrentTime.cpp:-222
> -

I don't like losing the following comments, which I found very illuminating about this routine.

Could you please copy these into your revised implementation so that non-Windows people are aware that Windows ticks are in 100s of nanoseconds, etc.?

[Patrick R. Gansterer]

Committed r120552: <http://trac.webkit.org/changeset/120552>

[Darin Adler]

This rewrite of the function was wrong. See bug 89433 for the problem and fix. But also, what was so wrong with the old version?

[Brent Fulgham]

Although there was a mistake in the units returned (milliseconds rather than seconds), I think the intent of the change was good because it reduced the code differences between the standard Windows and WinCE implementations.

There doesn't seem to be any meaningful difference in accurracy or precision between _ftime and GetSystemTimeAsFileTime, but since the routine is peppered with Windows-specific magic numbers related to "ticks per epoch" and so forth, it probably makes sense to stick with the Windows API calls so that the whole things hangs together.