Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW

Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-11-09 19:04:13

Thanks John for your contribution.
----- Original Message ----- 
From: "John Bytheway" <jbytheway+boost_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Tuesday, November 09, 2010 11:38 PM
Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW

> 
> On 05/11/10 17:25, Anthony Williams wrote:
>> Here are some questions you might want to answer in your review:
>> 
>> - What is your evaluation of the design?
> 
> It looks mostly well thought out and sensible, and I guess there's not
> much to say about the standardized portion, but I have a few concerns:
> 
> There are various places in the docs I see names used with a leading
> "__" (double underscore).  Is this intentional?  Such names are reserved
> for the use of the standard library, though I can see how they might be
> used in a library implementing standard behaviour.

I use __name__ for macros that expans to name with a link to the reference. It seems that I have writen 
__name_ too much times. I will correct them.

> There is various compile-time arithmetic happening implicitly in this
> library (e.g. in the many common_type return values).  Are integer
> overflows in this arithmetic detected and reported?  I think doing so
> would be valuable.

The boost ratio library avoids all kind of overflow that coudl resulting of arithmetic operation and can be simplified. The typedefs durations don't detect overflow. You will need a representation that handles with. I don't think the library should change the standard behavior, but could use representation the application gives for debug. Would this responds to your needs?

> This function puzzles me:
> 
> template <class Rep1, class Rep2, class Period>
> double operator/(const Rep1& s, const  duration<Rep2, Period>& d);

This overloading is used to get the frequency. I added it because I needed it for Boost.Stopwatch which report could report the frequency of an event.
 
> What is its intended purpose?  It seems to violate the type system
> somewhat by returning a double rather than an 'inverse duration'.  

What would you expect as result?

> Also
> it is defined in terms of another division:
> 
> CR(s)/ duration<CR, Period>(d)
>
> where CR is the common_type of Rep1 and Rep2.  I don't see where the
> meaning of *this* operation is defined.

You are right.  It should be

CR(s)/ duration<CR, Period>(d).count().

> You seem to be guaranteeing that high_resolution_clock is a typedef of
> one of the other two clocks?  Wouldn't it be better to leave open the
> possibility that it uses some other, more high-resolution clock?

Yes I could. I don't see nothing in the standard that forbid it. Do you have a better implementation for high-resolution_clock  and for which platform? I could include it if you have one.
 
>> - What is your evaluation of the documentation?
> 
> I have some low-level comments included at the end of this message, but
> more general things:
> 
> The docs say that the code fragments have an implicit "using namespace
> boost::chrono;".  In fact in practice many of them use the namespace
> explicitly or include "using namespace chrono;".  Could you use a
> namespace alias so that it is clear exactly which names are taken from
> this namespace.

This will take some time and will make the code and the documentation more weigth but I understand your concern, as it allows to see clearly what is part of the library. I will do it if other think it is better for the library.

> As others have said, I would appreciate a description of what the
> different clocks are.  In particular, I've always found 'monotonic' a
> bizarre adjective in this context; in what way is it more monotonic than
> the others?  (I think the answer is buried in the reference, under the
> definition of Clock::is_monotonic).

I have used the name of the standard. But maybe the name will change to steady_clock. A monotonic clock is a clock for which values of time_point never decrease as physical time advances as steady clock represent clocks for which values of time_point advance at a steady rate relative to real time. That is, the clock may not be adjusted.

The system_clock could be adjusted to a time_point that decresase the last time_point so is is not monotonic.

I will add the distinction in the documentation.
 
> Along similar lines, I think that somewhere (perhaps the FAQ or the
> rationale) should explain which APIs have been chosen to implement each
> clock on each platform, and why.  

I could add something to the Implementation Notes section.

> If there are any popular APIs for
> which clocks have not been written, are there plans to do so in the future?

I'm not aware of all the popular clocks API, but I don't know other than the provided by the library.
 
> I think it would be worth mentioning (probably in the FAQ) something
> about benchmarking, since that is a common requirement leading people to
> want access to clocks.  For example it could say which, if any, of the
> provided clocks are appropriate for benchmarking, and what to watch out
> for.  

Each clock has his own features. It depends on what do you need to benchmark. Most of the time, you could be interested in using a thread clock, but if you need to measure code subject to synchronization a process clock would be better. If you have a multi-process application, a system-wide clock will be needed.

> Some time ago Edward Grace proposed on this list some well thought
> out benchmarking techniques
> <http://article.gmane.org/gmane.comp.lib.boost.devel/191711>.  It would
> be nice if that or something like it found more prominence (but
> Boost.Chrono is not the place for that).

There is Boost.Stopwatch which will be reviewed 1Q/2Q 2011 that should cover the needs. Please take a look at and let me know if you fid there what you are looking for.
 
> I see hidden away among the examples a suggestion that the default
> duration silently ignores overflow.  This is what I expected, but even
> so I feel it deserves to be made more prominent earlier on (perhaps I
> missed it).

Yes, and you need to provide a representation that does someting else appropiated to your application. I will clarify this implicit fact as the default representation ignores overflow.

>> And finally, every review should answer this question:
>> 
>> - Do you think the library should be accepted as a Boost library?
> 
> Yes, but the docs need more work (both on the high level and in detail).
> All the reviewers have commented on this so far, and I recommend that
> when the documentation is revised it should be offered for comment again
> (preferably to be examined by people who didn't look at it this time :)).

Thanks a lot. I appreciate all your comment and expect they will help me to improve the library documentation and usability.

I will do it of course, but I think that it will be diffcult to find other volunters, so it will be great if you could review it again :).

I would comment your detailed improvements tomorrow.

Best regards,
Vicente