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

fast blackhole::localtime used gmtime_r, why not fast gmtime ? #182

Open
raidenluikang opened this issue Jan 22, 2018 · 2 comments
Open

Comments

@raidenluikang
Copy link

see blackhole/src/util/time.hpp:87, please.

I think that, there blackhole::gmtime , also.

@3Hren
Copy link
Owner

3Hren commented Feb 18, 2018

I don't remember exactly, but there was a bug in the implementation that was discovered during comparing our wheel with vanilla gmtime_r on the entire int32 range.

@Maturin
Copy link
Contributor

Maturin commented Nov 26, 2018

I started porting blackhole to Windows respectively Visual Studio 2017 and came also across the gmtime_r and localtime_r functions spread across the source code.

My best guess is that gmtime_r and localtime_r were used, because they are thread-safe respectively reentrant (_r) and gmtime is not.

It looks like that Visual Studio 2017 does not support the gmtime_r and the localtime_r functions, or at least I was not able to find them. Instead I found the C11 functions gmtime_s and localtime_s. Except for the order the arguments, they are compatible to the _r functions.

Right now I change the code always like this:

std::tm tm;
#ifdef _MSC_VER
    ::gmtime_s(&tm, &time);
#else
    ::gmtime_r(&time, &tm);
#endif 

But since :gmtime_s is C11, I'd like to solely use that.

Furthermore, blackhole/src/util/time.hpp seems to provide a project specific implementations for gmtime and localtime. Why aren't they used? That would be another options. Changing all calls to gmtime_r and localtime_r to use the blackhole functions.

Update: Altough C11 defines the _s time functions, they are not defined on my Linux box (Ubuntu). So only using them, is not going to work.

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

No branches or pull requests

3 participants