3Hren / blackhole

Yet another logging library.
MIT License
201 stars 57 forks source link

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

Open raidenluikang opened 6 years ago

raidenluikang commented 6 years ago

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

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

3Hren commented 6 years ago

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 commented 6 years ago

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.