-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: use uv_gettimeofday() #27029
src: use uv_gettimeofday() #27029
Conversation
uv_timeval_t ts; | ||
if (uv_gettimeofday(&ts) == 0) { | ||
writer.json_keyvalue("dumpEventTimeStamp", | ||
std::to_string(ts.tv_sec * 1000 + ts.tv_usec / 1000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might overflow when tv_sec
is a 32 bits long.
(I kind of regret that we didn't use int64_t
for uv_timeval_t.tv_sec
...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there's still time to change uv_gettimeofday
to uint64_t uv_gettimeofday()
(or unsigned long long int
) and always return microseconds? Seems like uv_timeval_t
is a little "legacy" for this API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a bad idea. Since uv_gettimeofday()
hasn't been released yet, there is time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss at libuv/libuv#2243.
@bnoordhuis I think I've addressed your nits with the new libuv release. |
CI: https://ci.nodejs.org/job/node-test-pull-request/22640/ EDIT(cjihrig): CI was green. |
dumpEventTimeStamp was not implemented on Windows, and did not include any error checking. This commit adds Windows support and error checking. PR-URL: nodejs#27029 Reviewed-By: Refael Ackermann <refack@gmail.com>
Use uv_gettimeofday() in GetCurrentTimeInMicroseconds() to remove the need for #ifdef logic. PR-URL: nodejs#27029 Reviewed-By: Refael Ackermann <refack@gmail.com>
Landed in 90cf2d5...8e1e994. |
Labeled as a WIP until the next libuv update (ignore the first commit here).
The next version of libuv will provide
uv_gettimeofday()
as a cross platform alternative togettimeofday()
.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes