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

Add decodelocaltime op #947

Closed
wants to merge 4 commits into from
Closed

Conversation

pmurias
Copy link
Contributor

@pmurias pmurias commented Aug 21, 2018

Will help us remove nqp::p6decodelocaltime.
Not tested on windows as I don't have a dev env set up on it :(

Copy link
Contributor

@bdw bdw left a comment

Choose a reason for hiding this comment

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

Please change localtime on windows to localtime_s. Other than that, ++


void MVM_platform_decodelocaltime(MVMint64 time, MVMint64 decoded[]) {
const time_t t = (time_t)time;
struct tm *tm = localtime(&t, &tm);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer the use of locatime_s here, which is equivalent to localtime_r in the POSIX world

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How portable across windows versions is localtime_s? I'm not a windows dev so I have no idea

Copy link
Contributor

Choose a reason for hiding this comment

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

From my (limited) understanding, exactly as portable as localtime because it is part of the 'Universal C Runtime Library'.

@pmurias
Copy link
Contributor Author

pmurias commented Sep 12, 2018

I fixed the localtime_s use. I don't use windows so I haven't tested the fix :(

@pmurias pmurias closed this Sep 12, 2018
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.

None yet

2 participants