On 10/11/10 00:04, vicente.botet wrote:
Thanks John for your contribution. ----- Original Message ----- From: "John Bytheway" <jbytheway+boost@gmail.com> To:
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 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?
Sorry, I don't follow. I think you're talking about runtime arithmetic here; I'm talking about compile-time arithmetic. For example uint64_t const big = uint64_t(1)<<62; typedef duration<uint64_t, ratio<1, big>> D1; typedef duration<uint64_t, ratio<1, big-1>> D2; D1 d1(0); D2 d2(0); auto d = d1+d2; Will the above compile? If so, what is the type of d? This is really a ratio library question; perhaps if I read the docs thereof it would tell me. But I'm concerned because of all the implicit combination of ratios in Boost.Chrono.
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.
OK, that makes sense. I can see why it's useful; it just makes me a little uneasy because the units of the return value depend on the units of d in a way that most of the operations strive to avoid. If you added it for Boost.Stopwatch do I take it that it's not in the standard proposal?
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?
Unless you returned something from Boost.Units there really isn't a sensible option (and I'm certainly not suggesting you return something from Boost.Units).
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.
For Windows there's QueryPerformanceCounter and QueryPerformanceFrequency that people are always suggesting for use in Boost.Timer. I've never used them, but I get the impression they're higher precision. OTOH, I think that their precision is not known at compile-time, which may be troublesome. I believe there are similar things on other platforms. One thing I would like is a common interface akin to libfftw's getticks. (See the "Cycle Counters" part of http://www.fftw.org/download.html). This is also in arbitrary units, not seconds, though, and I'm not sure how many of the underlying APIs have a known compile-time precision. So perhaps all these things do not belong in Boost.Chrono.
- What is your evaluation of the documentation?
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.
That's reasonable :).
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.
Are you saying the standardised name might change?
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.
Thanks. It puzzled me for years :).
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.
Cool.
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.
Yes, that kind of information is worth putting n the FAQ, I think.
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 tried, but the documentation link on LibrariesUnderConstruction doesn't seem to work (I get a blank page).
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.
You're welcome :).
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 doubt I could as well a job on a second reading, but perhaps I could try... John Bytheway