Re: [boost] [chrono] Possible bug in boost::chrono::process_real_cpu_clock::now() under Windows 32bits

Subject: Re: [boost] [chrono] Possible bug in boost::chrono::process_real_cpu_clock::now() under Windows 32bits
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-01-05 11:53:12

Le 05/01/12 17:38, Howard Hinnant a écrit :
> On Jan 5, 2012, at 10:30 AM, ivan.lelann_at_[hidden] wrote:
>
>> Hi,
>>
>> As of 1.48 Boost.Chrono contains code below for boost::chrono::process_real_cpu_clock::now()
>> (boost\chrono\detail\inlined\win\process_cpu_clocks.hpp)
>>
>>     clock_t c = ::clock();
>>     /* ... */
>>     return time_point(
>>       duration(c*(1000000000l/CLOCKS_PER_SEC))
>>     );
>>
>> duration::rep is int64/nanoseconds, clock_t is long. This is VS2008, Win XP 32bits.
>>
>> I think "c" should be cast to duration::rep before being multiplied.
>> I randomly get negative time_point and this seems to be fixed by replacing
>> "c" with "((duration::rep) (c))" in duration constructor :
>>
>>         duration( ((duration::rep) (c)) *(1000000000l/CLOCKS_PER_SEC))
>>
>> Can someone confirm my view and that the fix is correct ?
> Your fix looks correct to me.  I'm not positive what value CLOCKS_PER_SEC has on your platform.  But here is a reformulation of your solution that might be a little safer (depending on what CLOCKS_PER_SEC actually is):
>
> typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R;
> return time_point(
>    duration(static_cast<rep>(c)*R::num/R::den)
> );
>
> Using giga avoids mistakes in counting zeros.
>
> Using ratio_divide reduces the fraction 1000000000l/CLOCKS_PER_SEC to lowest terms, no matter the value of CLOCKS_PER_SEC.
>
> You can use rep as a shortcut for duration::rep, assuming this is inside of the clock's now() function.
>
> static_cast<rep>  is a little safer than a C cast.  I would expect the results to be the same, but it is good to code defensively.
>
> If you know a-priori that R::den is 1, you might:
>
> typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R;
> static_assert(R::den == 1, "oops!");
> return time_point(
>    duration(static_cast<rep>(c)*R::num)
> );

Hi,

yes this seems much more safer.

Ivan please, could you create a ticket for this issue?

Best,
Vicente