-
Notifications
You must be signed in to change notification settings - Fork 166
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
Remove call of localtime #3246
Remove call of localtime #3246
Conversation
The ftime handling of time-zones and DST is not clear to me, and thus I am not sure if it is worth the effort. In detail: as far as I can tell POSIX has removed ftime, and Microsoft does in In particular it is not clear if it is just a flag, or whether other values that 0 and 1 are intended for odd cases (two hour DST, 20 minute DST etc). (Oh, and for POSIX it isn't even clear if the DST-flag indicates the presence of DST some time during the year or whether is currently active.) |
Wouldn't it just be easier to copy the tm struct to avoid the race condition? |
It's about the two different calls to timing functions:
I am not sure if relevant at all or if the proposed changes are good enough, but what I did:
|
What is the status of this one?
|
46daec6
to
42ca6cc
Compare
Thanks for reminding me.
There is no milliseconds or microseconds in struct tm. Therefore there are two different calls to determine current time in current code. This should be avoided. In the PR I also test for dstflag != 0 only.
The dstflag is nonzero if DST is currently in effect for the local time zone. That is what I tested in DST (now) and 7 months ago (when DST was not in effect).
Yes. gettimeofday is used in the PR for that reason.
I tested successfully in CET with and without DST in effect.
It is used as a flag.
Yes, would be good to get some feedback from POSIX expert @sjoelund if the code is formally correct and if it can be successfully tested. |
@HansOlsson @sjoelund Any ideas how to proceed? |
I think it is ok, but want to hear from @sjoelund |
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.
Ok, then.
@beutlich Is there a reason to not merge this? (Apart from resolving the conflict). |
42ca6cc
to
ff37ed1
Compare
Done. |
@beutlich can you update this from master? I did it like 7 minutes ago, but it is still out of sync and it invalidates my review. |
There is a race condition between localtime and ftime/gettimeofday in ModelicaInternal_getTime. In case ftime/gettimeofday is called at a very new second, this can lead to a inaccuracy of (almost) 1 second for the calculated time. Using gmtime, there is no need to call localtime. Instead the struct tm can be directly computed from the obtained timebuffer and timezone information.
6b7a6f2
to
f9e70d3
Compare
Done once again. |
There is a race condition between localtime and ftime/gettimeofday in ModelicaInternal_getTime. In case ftime/gettimeofday is called at a very new second, this can lead to an inaccuracy of (almost) 1 second for the calculated time.
Using gmtime, there is no need to call localtime. Instead the struct tm can be directly computed from the obtained timebuffer and timezone information.