Skip to content

Revise implementation of jerry_port_get_local_time_zone_adjustment on Win32 #4589

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 14, 2024
Merged

Revise implementation of jerry_port_get_local_time_zone_adjustment on Win32 #4589

merged 1 commit into from
Nov 14, 2024

Conversation

lygstate
Copy link
Contributor

Handle is_utc and not is_utc properly.
Handle the limitation of Win32 date API properly.

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]

@lygstate lygstate force-pushed the win32-time-zone branch 3 times, most recently from 6e0245c to 369dacf Compare February 10, 2021 10:03
akosthekiss
akosthekiss reviewed Feb 10, 2021
View reviewed changes
jerry-port/default/default-date.c Outdated
* https://docs.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-filetime
*/
static const long long UnixEpochOfDate_1601_01_02 = -11644387200000LL; /* unit: ms */
static const long long UnixEpochOfDate_30827_12_29 = 9106702560000000LL; /* unit: ms */
Copy link
Member

Choose a reason for hiding this comment

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

Why not LONGLONG here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not LONGLONG here?

UnixEpoch, LONGLONG are windows specific

Copy link
Member

Choose a reason for hiding this comment

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

And this is Windows-specific code path. BTW, this code (copied from below)

  if (unix_ms < UnixEpochOfDate_1601_01_02)
  {
     unix_ms = UnixEpochOfDate_1601_01_02;
  }
  if (unix_ms > UnixEpochOfDate_30827_12_29)
  {
     unix_ms = UnixEpochOfDate_30827_12_29;
  }

is working with doubles and LONGLONGs (or long longs). It might be wise to add casts explicitly as in UnixTimeMsToFileTime and FileTimeToUnixTimeMs.

jerry-port/default/default-date.c Outdated
@@ -57,30 +69,59 @@ static double FileTimeToUnixTimeMs (FILETIME ft)
* available. Otherwise, returns 0, assuming UTC time.
*/
double jerry_port_get_local_time_zone_adjustment (double unix_ms, /**< ms since unix epoch */
bool is_utc) /**< is the time above in UTC? */
bool is_utc) /**< is the time above in UTC, */
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary edit. Question mark was OK.

jerry-port/default/default-date.c Outdated
static const LONGLONG TicksPerMs = 10000; /* 1 tick is 100 nanoseconds */

/**
Copy link
Member

Choose a reason for hiding this comment

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

/* instead of /** as this is not a doxygen comment

jerry-port/default/default-date.c Outdated
SYSTEMTIME utcSystemTime, localSystemTime;
bool timeConverted = false;

/**
Copy link
Member

Choose a reason for hiding this comment

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

/* instead of /** as this is not a doxygen comment

jerry-port/default/default-date.c Outdated
* query time zone adjustment, this date(1601-01-02) will make sure both utc/local
* time succeed with Win32 API, using date(1601-01-01) may leading win32 api
* failure, as after convert between local time/utc time, the time may earlier than
* date 1601-01-01 in utc time, that's exceed the FILETIME representation range.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion, edited for language: If the time is earlier than the date 1601-01-02, then always using date 1601-01-02 to query time zone adjustment. This date (1601-01-02) will make sure both UTC and local time succeed with Win32 API. The date 1601-01-01 may lead to a win32 api failure, as after converting between local time and utc time, the time may be earlier than 1601-01-01 in UTC time, that exceeds the FILETIME representation range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

jerry-port/default/default-date.c Outdated
UnixTimeMsToFileTime (unix_ms, &fileTime);
/* If time is earlier than year 1601, then always using year 1601 to query time zone adjustment */
if (fileTime.dwHighDateTime >= 0x80000000)
/* Same reason as upper, do not use the latest support day */
Copy link
Member

Choose a reason for hiding this comment

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

suggestion, edited for language: Like above, do not use the last supported day

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both upated.

@lygstate lygstate force-pushed the win32-time-zone branch 2 times, most recently from c2b6aff to c6be129 Compare February 10, 2021 12:29
akosthekiss
akosthekiss reviewed Feb 10, 2021
View reviewed changes
Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

I'm still not really happy about long longs, such inconsistency just itches me. The rest looks OK with me -- in general. But the details of computations should be checked by Windows-focused reviewers.

@lygstate
Copy link
Contributor Author

I'm still not really happy about long longs, such inconsistency just itches me. The rest looks OK with me -- in general. But the details of computations should be checked by Windows-focused reviewers.

OK, then LONGLONG

@lygstate lygstate requested a review from akosthekiss March 24, 2021 05:33
@lygstate
Copy link
Contributor Author

ping for review or merge

zherczeg
zherczeg approved these changes Mar 30, 2021
View reviewed changes
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@lygstate
Copy link
Contributor Author

any progress?

@lygstate lygstate requested review from zherczeg and rerobika July 30, 2021 19:53
@lygstate lygstate force-pushed the win32-time-zone branch 2 times, most recently from 4a38d6c to b629b5b Compare November 13, 2024 20:37
@lygstate
Revise implementation of jerry_port_get_local_time_zone_adjustment on…
da3d472 
… Win32

Handle the limitation of Win32 date API properly.

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]
robertsipka
robertsipka approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@robertsipka robertsipka left a comment

Choose a reason for hiding this comment

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

LGTM, conflicts have been resolved, it was previously accepted by 2 reviewers, but it was abandoned, I don't see why it couldn't be merged. @LaszloLango please double check and merge it if it looks good to you as well.

@LaszloLango LaszloLango merged commit d2e0d71 into jerryscript-project:master Nov 14, 2024
26 checks passed
@lygstate lygstate deleted the win32-time-zone branch November 14, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertsipka robertsipka robertsipka approved these changes

@LaszloLango LaszloLango LaszloLango approved these changes

@ossy-szeged ossy-szeged Awaiting requested review from ossy-szeged

@akosthekiss akosthekiss Awaiting requested review from akosthekiss

@zherczeg zherczeg Awaiting requested review from zherczeg

@rerobika rerobika Awaiting requested review from rerobika

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lygstate @zherczeg @robertsipka @akosthekiss @LaszloLango @rerobika