[Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
Hi everyone, The review of Vicente Botet's Chrono library starts TOMORROW (November 6th) and lasts until November 15th unless an extension occurs. What is it? ======== Boost.Chrono aims to implement the new time facilities in C++0x, as proposed in N2661 - A Foundation to Sleep On (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2661.htm). That document provides background and motivation for key design decisions and is the source of a good deal of information in this documentation. See the current C++0x working draft at http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3126.pdf for details of what was accepted. The Boost.Chrono library provides: * A means to represent time durations: managed by the generic duration class . Examples of time durations include days, minutes, seconds and nanoseconds, which can be represented with a fixed number of clock ticks per unit. All of these units of time duration are united with a generic interface by the duration facility. * A type for representing points in time: time_point. A time_point represents an epoch plus or minus a duration. The library leaves epochs unspecified. A time_point is associated with a clock. * Several clocks, some of which may not be available on a particular platform: system_clock, monotonic_clock and high_resolution_clock. A clock is a pairing of a time_point and duration, and a function which returns a time_point representing now. * To make the timing facilities more generally useful, Boost.Chrono provides a number of additional clocks that are thin wrappers around the operating system's time APIs, thereby allowing the extraction of read (wall clock) time, user CPU time, system CPU time, - process_real_cpu_clock, captures real (wall clock) CPU times. - process_user_cpu_clock, captures user-CPU times. - process_system_cpu_clock, captures system-CPU times. - A tuple-like class __process_cpuclock_, that captures real, user-CPU, and system-CPU times together. - A thread_clock thread monotonic clock, when supported by a platform. * Boost.Chrono also includes typeof registration for duration and time_point to permit using emulated auto with C++03 compilers. * Boost.Chrono also provides I/O for duration and time_point. It builds on <boost/ratio/ratio_io.hpp> to provide readable and flexible formatting and parsing for types in <boost/chrono.hpp>. The duration unit names can be customized through a new facet: duration_punct. Getting the library ============= The latest version of this library may be downloaded from vault: http://www.boostpro.com/vault/index.php?action=downloadfile&filename=chrono.zip&directory=System& SVN: http://svn.boost.org/svn/boost/sandbox/chrono Boost.Chrono depends on Boost.Ratio http://www.boostpro.com/vault/index.php?action=downloadfile&filename=ratio.zip&directory=Math%20-%20Numerics&PHPSESSID=2d7859f3e812c993f5a9e2d9900dfee8 and optionaly Boost.System Header Only http://www.boostpro.com/vault/index.php?action=downloadfile&filename=system.zip&directory=System&PHPSESSID=2d7859f3e812c993f5a9e2d9900dfee8. and the docs may be viewed here HTML: http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/index.htm... PDF: http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/chrono.pdf You can get all these information also (which should avoid some long links) from http://svn.boost.org/trac/boost/wiki/LibrariesUnderConstruction#Boost.Chrono Writing a review ================ If you feel this is an interesting library, then please submit your review to the developer list (preferably), or to the review manager. Here are some questions you might want to answer in your review: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study? - Are you knowledgeable about the problem domain? And finally, every review should answer this question: - Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Anthony Review Manager for the proposed Boost.Chrono library -- Author of C++ Concurrency in Action http://www.stdthread.co.uk/book/ just::thread C++0x thread library http://www.stdthread.co.uk Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK. Company No. 5478976
----- Original Message ----- From: "Anthony Williams" <anthony.ajw@gmail.com> To: <boost@lists.boost.org> Sent: Friday, November 05, 2010 6:25 PM Subject: [boost] [Review] Formal Review of Proposed Boost.Chrono LibraryStarts TOMORROW
The Boost.Chrono library provides:
* Boost.Chrono also provides I/O for duration and time_point. It builds on <boost/ratio/ratio_io.hpp> to provide readable and flexible formatting and parsing for types in <boost/chrono.hpp>. The duration unit names can be customized through a new facet: duration_punct.
Hi all, unfortunately the implementation for parsing input streams used a scan_keyword.hpp adapted from llvm/libc++ which was subject to a license that is not compatible with Boost. I have removed temporarly the dependency on this file, so there will be no license issues, but this has as consequence that there is no parsing at all now. I'm working on a re-implementation of the scan keywords algorithm that at the end should be even more efficient than the original one (at least this is what I hope). Until that the library will provide only formatting but not parsing. Hoping you could wait for a Boost compatible implementation. Best regards, Vicente
On 11/05/2010 06:25 PM, Anthony Williams wrote:
If you feel this is an interesting library, then please submit your review to the developer list (preferably), or to the review manager.
It certainly is an interesting library :-)
Here are some questions you might want to answer in your review:
- What is your evaluation of the design?
Looks good to me
- What is your evaluation of the implementation?
Clean code, good to read.
- What is your evaluation of the documentation?
I am still having some trouble with the documentation. For instance, one of the coolest things about Chrono is the thread_clock, IMO. So I looked for thread_clock in the documentation. After being mentioned in the Description section, the next instance is this section in the tutorial: "How to get the actual CPU milliseconds (or other units?) used by the current thread between end and start?" However, although thread_clock is mentioned in the text, it is not to be found in any code sample. I know that it is easy to use it, but still, a few lines of code proving it would certainly not hurt. The next section mentioning the thread_clock is the reference section. It contains this description: "thread_clock class provides access to the real thread wall-clock, i.e. the real CPU-time clock of the calling thread." Huh? This is confusing at best, I'd say. I hope it measures the CPU-time for the calling thread, but not the wall clock. The wall clock should be the same for all threads.
- What is your evaluation of the potential usefulness of the library?
Very useful! For me, the thread_clock is the most relevant part. It allowed me to do rather simple but invaluable performance analysis in a multithreaded pipeline application.
- Did you try to use the library? With what compiler? Did you have any problems?
I used the previous version in a production scenario. No problems. gcc-4.4.3, Ubuntu 10.4, 64bit
- How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study?
I concentrated on the thread_clock (who would have guessed :-) ). I've been a user of this library for several months now, have contributed patches to the code and given feedback to the documentation. I'd call it a partial in-depth study ;-)
- Are you knowledgeable about the problem domain?
Partially, at least
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
Yes, but the documentation should receive another update. Regards, Roland
Hi, thanks for taking time on the review. ----- Original Message ----- From: "Roland Bock" <rbock@eudoxos.de> To: <boost@lists.boost.org> Sent: Monday, November 08, 2010 6:44 AM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
- What is your evaluation of the documentation?
I am still having some trouble with the documentation. For instance, one of the coolest things about Chrono is the thread_clock, IMO. So I looked for thread_clock in the documentation.
After being mentioned in the Description section, the next instance is this section in the tutorial: "How to get the actual CPU milliseconds (or other units?) used by the current thread between end and start?" However, although thread_clock is mentioned in the text, it is not to be found in any code sample. I know that it is easy to use it, but still, a few lines of code proving it would certainly not hurt.
I will add a more explicit use of thread_clock in the documentation and a concrete example.
The next section mentioning the thread_clock is the reference section. It contains this description:
"thread_clock class provides access to the real thread wall-clock, i.e. the real CPU-time clock of the calling thread."
Huh? This is confusing at best, I'd say. I hope it measures the CPU-time for the calling thread, but not the wall clock. The wall clock should be the same for all threads.
Oh, you are right. I will replace by "thread_clock class provides access to the CPU-time spent by the calling thread."
- What is your evaluation of the potential usefulness of the library?
Very useful! For me, the thread_clock is the most relevant part. It allowed me to do rather simple but invaluable performance analysis in a multithreaded pipeline application.
Glad to hear it.
- Do you think the library should be accepted as a Boost library?
Yes, but the documentation should receive another update.
Many thanks. Please, could you point to other documentation improvements? Best, Vicente
On 11/08/2010 06:32 PM, vicente.botet wrote:
Yes, but the documentation should receive another update.
Many thanks. Please, could you point to other documentation improvements?
Hi Vicente, here are some more items. I'm sure there's more, but I'm in a bit of a hurry and did not go through the document line by line: http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr... *) "the extraction of read (wall clock) time," <= real? CPU time? *) "|process_real_cpu_clock| <http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chrono/reference/other_clocks.html#boost_chrono.reference.other_clocks.process_cpu_clocks_hpp.process_real_cpu_clock>, captures real (wall clock) CPU times." <= is this really CPU time? Isn't it rather real process time? *) "A tuple-like class __process_cpuclock_, that" <= The link is broken. http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr... I'd prefer to see the same sequence of duration, timepoint, clocks, other clocks etc. Currently every chapter is different: - Description: duration, time_point, clocks, other clocks, io - Tutorial: duration, clocks, time_point, io - Examples: time_point, duration, clocks, conversions, reporting - Reference: duration, time_point, clocks, io, other clocks Personally, I like Description and Reference best. http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr... Taking a look at the table of contents for this section shows that duration is covered in much detail, time_point, a bit less. Clock and IO seem to be rather neglected. Given the sheer number of different clocks available in Chrono, this seems unbalanced. And taking a closer look at the "Clocks" section shows: The tutorial explains the interface of a clock, but not the clocks themselves. What is a system clock, what's so cool about a monotonic_clock, what distinguishes a high_precision_clock (another damaged link here). In order to remove confusion about wall time and CPU time, a small example using different clocks might be useful (if it is there, I missed it). I would like to see this as part of the tutorial. http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr... - Just stumbled over this: simaulation - There is no thread_clock mentioned here, even though there is a thread interface demonstration program. Regards, Roland
----- Original Message ----- From: "Roland Bock" <rbock@eudoxos.de> To: <boost@lists.boost.org> Sent: Monday, November 08, 2010 9:03 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
On 11/08/2010 06:32 PM, vicente.botet wrote:
Many thanks. Please, could you point to other documentation improvements?
here are some more items. I'm sure there's more, but I'm in a bit of a hurry and did not go through the document line by line:
Please if you have sometime later on, I would apreciate if you can make a deep review of the documentation, better if you do it before the review, but I would take them in account even later. http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr...
*) "the extraction of read (wall clock) time," <= real? CPU time?
OK.
*) "|process_real_cpu_clock| OK. <http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chrono/reference/other_clocks.html#boost_chrono.reference.other_clocks.process_cpu_clocks_hpp.process_real_cpu_clock>, captures real (wall clock) CPU times." <= is this really CPU time? Isn't it rather real process time?
Yes, is the time since the process start-up. I will clarify this point.
*) "A tuple-like class __process_cpuclock_, that" <= The link is broken. OK.
http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr... I'd prefer to see the same sequence of duration, timepoint, clocks, other clocks etc. Currently every chapter is different:
- Description: duration, time_point, clocks, other clocks, io - Tutorial: duration, clocks, time_point, io - Examples: time_point, duration, clocks, conversions, reporting - Reference: duration, time_point, clocks, io, other clocks
Personally, I like Description and Reference best.
OK. I will make them uniform and take the Description order: duration, time_point, clocks, other clocks, io http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr...
Taking a look at the table of contents for this section shows that duration is covered in much detail, time_point, a bit less. Clock and IO seem to be rather neglected. Given the sheer number of different clocks available in Chrono, this seems unbalanced. And taking a closer look at the "Clocks" section shows: The tutorial explains the interface of a clock, but not the clocks themselves. What is a system clock, what's so cool about a monotonic_clock, what distinguishes a high_precision_clock (another damaged link here).
You are right, the documentation pre-suposse that the user is familiar with the different kind of clocks. An explanation of when each clock could be used seems necessary. The standard defines tree system-wide clocks that are associated to the computer time. The system_clock represents system-wide realtime clock that can be synchronized with an external clock. monotonic_clock can not be changed explicitly and the time since the initial epoch increase in a monotonic way. high_resolution_clock intend to use the system-wide clock provided by the platform with the higest resolution. The library adds some clocks that are specific to a process or a thread, that is there is a clocks per process or per thread. System clocks are useful when you need to correlate the time with a known epoch so you can convert it to a calendar time. Note the specific functions in the system_clock class. Monotonic clocks are useful when you need to wait for a specific amount of time. As system_clock time can be reset, timers will not be safe. The clocks used for the Thread library should be monotonic, but the platform don't always provide monotonic thread interfaces. When available, high resolution clocks are usualy more expensive than the other system-wide clocks, so they are used only when the provided resolution is required to the application. I use process clocks and thread clocks to measure the time spent by them, for example they can be used to basic time-spent profiling of different blocks of code (Boost.Stopwatch is clear example of this use) or by regulation mechanism. I will include something that makes clear the distiction.
In order to remove confusion about wall time and CPU time, a small example using different clocks might be useful (if it is there, I missed it). I would like to see this as part of the tutorial.
No, you don't missed it. I will add a specific section for each clock. http://svn.boost.org/svn/boost/sandbox/chrono/libs/chrono/doc/html/boost_chr...
- Just stumbled over this: simaulation - There is no thread_clock mentioned here, even though there is a thread interface demonstration program.
The intent of the simulation was not to show how thread cloks can be used but how the Thread interface can be modified to manage with a generic and scalable clock interface. Thanks, Vicente
On 08/11/10 21:36, vicente.botet wrote:
----- Original Message ----- From: "Roland Bock" <rbock@eudoxos.de>
On 11/08/2010 06:32 PM, vicente.botet wrote:
Many thanks. Please, could you point to other documentation improvements?
here are some more items. I'm sure there's more, but I'm in a bit of a hurry and did not go through the document line by line:
Please if you have sometime later on, I would apreciate if you can make a deep review of the documentation, better if you do it before the review, but I would take them in account even later.
FYI, I'm in the midst of a deeper review, but I probably won't finish tonight. John Bytheway
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. 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. This function puzzles me: template <class Rep1, class Rep2, class Period> double operator/(const Rep1& s, const duration<Rep2, Period>& d); What is its intended purpose? It seems to violate the type system somewhat by returning a double rather than an 'inverse duration'. 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 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?
- What is your evaluation of the implementation?
I didn't look.
- 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. 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). 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. If there are any popular APIs for which clocks have not been written, are there plans to do so in the future? 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. 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). 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).
- What is your evaluation of the potential usefulness of the library?
High. I'm thoroughly fed up of the vast array of heterogeneous C clock interfaces.
- Did you try to use the library? With what compiler? Did you have any problems?
Didn't try.
- How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study?
I read the docs in detail, but nothing else.
- Are you knowledgeable about the problem domain?
Not particularly.
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 :)). Detailed documentation comments follow. They refer to the HTML docs. They overlap with some other reviewers' comments, and some I've given in reference to certain pages also apply to subsequent pages. Where I have asked a question I may be implying that you should not only answer me but also put the answer in the docs. Overview: - "Replaceable text that you will need to supply is in italics" is a bit self-contradictory; I suggest rather "Text for which you will need to supply a replacement is in italics". - "free_function" isn't monospaced, when the text implies it is. Motivation: - The phrase "allowing the extraction of read (wall clock) time" is not clear to me. Is there such a thing as "read time"? (From later pages I guess it should be "real", I don't really like this term either, but I have no better suggestion.) Description: - "__process_cpuclock_" seems to be incorrectly underlined. And does the name really start with two underscores? Users' Guide: - Title is missing a space. Getting started: - s/Exceptions safety/Exception safety/ - On thread safety, could you clarify how unsafe they are (when not otherwise specified). e.g. Only one thread may use boost::chrono at all, or each function may be used in only one thread at a time. - By "not lastly" do you mean "not recently"? - In the hello world program s/tooks/took/ Tutorial: - I would remove all the apostrophes from plurals ("duration's", "time_point's"). - "__high_precisionclock_" again looks mis-underlined and I guess doesn't really start with "__". - Rather than "Clock::duration d = t1 - Clock::now();" I would have expected a more common use case to be "Clock::duration d = Clock::now() - t1;" (as in the tutorial). Is that what was intended? - There are some spaces in the code after "boost::chrono::", e.g. in "boost::chrono:: milliseconds"; were they deliberate? - In 'with the proper spellings for hours, "minutes" and "seconds"', should 'hours' be in double-quotes also? - The third example in I/O has five lines sending output to cout, but only two lines in the sample output. How is this? Examples: - Is the min function mentioned here part of the library, or simply something a user might write? It's not clear. - The zero_default example is a bit intimidating. Might it be less so if you took advantage of Boost.Operator to define most of the operators? - A round-up function appears here which is very similar to but not quite the same as the one in the Tutorial. Could one suffice? - Does the "tiny program that times how long until a key is struck" really work as the content suggests? I would expect line-buffered input on my machine, and thus the only key which will cause cin.get() to return would be Enter. - s/simaulation/simulation/ Reference - C++0x: - s/constexpr don't used/constexpr isn't used/ - s/when appropiated/where appropriate/ - You say constexpr isn't used, but I see e.g. BOOST_CHRONO_CONSTEXPR macro being used. Will this automagically expand to constexpr once Boost.Config indicates support? (I see this is mentioned again in appendix F, but it's still not clear) - s/The default vaule behavior is as BOOST_CHRONO_USES_ARRAY_ASSERT was defined/The default behavior is as if BOOST_CHRONO_USES_ARRAY_ASSERT was defined/ - s/texte/text/ - s!files in detail/win!files in boost/detail/win! - You are sometimes capitalizing Non-Member for no apparent reason. - It would help if the not-really-code parts of the reference (such as the "see below" in common_type) were somehow distinguished; most Boost docs use italics for this. - duration member function count says "Returns: `rep_v."; should it be "Returns: rep_"? I see at least one other stray backtick/v pair on this page. - The documentation of the pre-/post- inc-/dec- rement operators is a bit broken. All the headings are for inc-, not dec-, and one of the "++"s is not consistently formatted. - Functions with BOOST_CHRONO_CONSTEXPR in the duration summary are not consistently presented in the details; the non-static ones have BOOST_CHRONO_CONSTEXPR, max() has constexpr, and zero() and min() have nothing at all. - I don't see the definition of "milli", "micro", etc in the reference. I guess they're defined in Boost.Ratio. I think this warrants a mention. - s/classes and non-member function./classes and non-member functions./ - The heading above the docs for operator-(const time_point<Clock, Duration1>& lhs, const time_point<Clock, Duration2>& rhs); is wrong. - If BOOST_CHRONO_HAS_CLOCK_MONOTONIC is false, can the user expect monotonic_clock to be defined? - Can you say anything about time zones wrt system_clock (i.e. will it be local time or UTC, or is that undefined)? Reference - Other clocks: - How do the process clocks handle threads and child processes? Is this defined? Appendix A: History: - This is empty; should it be? Appendix B: Rationale: - Should the URL of N2661 be a clickable link? It isn't. - s/Current N3000 doesn't allows to copy-construct or assign ratio instances/Current N3000 doesn't allow programs to copy-construct or assign instances/ - Unmatched bracket in "([LWG 1281)" Appendix C: - Also empty Appendix D: - s/sometimes gives/sometimes give/ Congratulations on reaching review! John Bytheway
Thanks John for your contribution. ----- Original Message ----- From: "John Bytheway" <jbytheway+boost@gmail.com> To: <boost@lists.boost.org> 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
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
----- Original Message ----- From: "John Bytheway" <jbytheway+boost@gmail.com> To: <boost@lists.boost.org> Sent: Wednesday, November 10, 2010 8:40 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
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.
I wasn't clear about what overflow you were talking so for ratio the answers is yes.
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?
No. there is an overflow. The following compile-time error is obtained:
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.
I understand.
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?
Yes :(.
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 are right. Whithout the correct units the result has no sense. For coherency, I think that I should be forced to remove this overloading, and let Stopwatch make his internal computation.
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 forbids 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.
Boost.chrono uses already them. I will add the following table as requested. "The following table presents a resume of which API is uused for each clock on each platform [table Clock API correspondence [[Clock] [Windows Platform] [Posix Platform] [Mac Platform] ] [[__system_clock__] [GetSystemTimeAsFileTime] [clock_gettime( CLOCK_REALTIME)] [gettimeofday] ] [[__monotonic_clock__] [QueryPerformanceCounter and QueryPerformanceFrequency] [clock_gettime( CLOCK_MONOTONIC)] [mach_timebase_info,mach_absolute_time] ] [[__process_*_cpu_clock__] [GetProcessTimes] [times] [times] ] [[__thread_clock__] [GetThreadTimes] [pthread_getcpuclockid] [pthread_getcpuclockid (Not tested)] ] ] "
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.
The way to manage with precission at run-time, is to state an desired precission at compile-time and convert at run-time. This means that if the run-time resolution is higher than the compile-time one precission is lost. The current implementation for windows set the Clock duration unit to nanoseconds.
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.
Without the processor speed the getticks() function is not of utility for the Chrono framework. If the user know the processor speed in Mhz at compile-time s/he can define a cycle_count_clock in a simple way as follows: template <long long speed> struct cycle_count_clock { typedef typename boost::ratio_multiply<boost::ratio<speed>, boost::mega>::type frequency; // Mhz typedef typename boost::ratio_divide<boost::ratio<1>, frequency>::type period; typedef long long rep; typedef boost::chrono::duration<rep, period> duration; typedef boost::chrono::time_point<cycle_count_clock> time_point; static time_point now() { // return exact cycle count return time_point(duration(::getticks())); // access to clock cycle count } }; and use it as follows on a 1600 MHz processor: typedef cycle_count_clock<1600> clock; There is an example that uses an emmulated getticks and uses this kind of clock.
So perhaps all these things do not belong in Boost.Chrono.
- What is your evaluation of the documentation? 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?
The standard is no finished yet (Mars 2011?). So yes, the standard can change. It is not thatthey will change the name of the class but add a new one with a different semantic and remove the monotonic_clock. This doesn't means that Boost.Chrono will break compatibility if the standard change. I will just add the new clock.
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.
Added.
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).
I don't get blank page. Please could you try http://svn.boost.org/svn/boost/sandbox/chrono/libs/stopwatches/doc/html/inde...
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...
Thanks anyway, Vicente
On 11/11/10 09:54, vicente.botet wrote:
From: "John Bytheway" <jbytheway+boost@gmail.com>
On 10/11/10 00:04, vicente.botet wrote: <snip> 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?
No. there is an overflow. The following compile-time error is obtained:
Good.
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 are right. Whithout the correct units the result has no sense. For coherency, I think that I should be forced to remove this overloading, and let Stopwatch make his internal computation.
Thanks; that makes me feel better :).
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 forbids 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.
Boost.chrono uses already them. I will add the following table as requested. <snip>
Cool. For POSIX process/thread times, I note the existence of clock_gettime(CLOCK_PROCESS_CPUTIME_ID) and clock_gettime(CLOCK_THREAD_CPUTIME_ID) which are possibly candidates for the process / thread timings. At the same time, their man page says they're unreliable on SMP systems, so probably not useful.
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.
Without the processor speed the getticks() function is not of utility for the Chrono framework. If the user know the processor speed in Mhz at compile-time s/he can define a cycle_count_clock in a simple way as follows:
Right; I see that. My point was that the implementation of getticks itself has lots of platform-specific code in it, and something equivalent might be usefully added to Boost (but probably not in Boost.Chrono).
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).
I don't get blank page. Please could you try http://svn.boost.org/svn/boost/sandbox/chrono/libs/stopwatches/doc/html/inde...
Yes, that works. Thanks. John
On 11/11/10 20:40, John Bytheway wrote:
Right; I see that. My point was that the implementation of getticks itself has lots of platform-specific code in it, and something equivalent might be usefully added to Boost (but probably not in Boost.Chrono).
Can't boost.Chrono concept wraps aroudn the concept of cycles and providde a wrapper around rdtsc adn its ilk ? Or is it more a work for StopWatch ?
----- Original Message ----- From: "joel falcou" <joel.falcou@lri.fr> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 9:25 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
On 11/11/10 20:40, John Bytheway wrote:
Right; I see that. My point was that the implementation of getticks itself has lots of platform-specific code in it, and something equivalent might be usefully added to Boost (but probably not in Boost.Chrono).
Can't boost.Chrono concept wraps aroudn the concept of cycles and providde a wrapper around rdtsc adn its ilk ? Or is it more a work for StopWatch ?
Sorry for the late response, I missed this post. As I said in another post it is very simple for the user to define a clock based on whatever cycle count the user can provide: template <long long speed> struct cycle_count_clock { typedef typename boost::ratio_multiply<boost::ratio<speed>, boost::mega>::type frequency; // Mhz typedef typename boost::ratio_divide<boost::ratio<1>, frequency>::type period; typedef long long rep; typedef boost::chrono::duration<rep, period> duration; typedef boost::chrono::time_point<cycle_count_clock> time_point; static time_point now() { // return exact cycle count return time_point(duration(provided_by_the_user::getticks())); // access to clock cycle count } }; Boost.Chrono could provide sych a class, but the getticks() function should be provided by the user. If someone is able to provide a portable getticks() function (I'm myself unable to do it) and include it in Boost, Boost.Chrono will definitely use it to provide this cycle_count_clock. StopWatch is at high level and builds on top of Boost.Chrono. Best, Vicente
----- Original Message ----- From: "vicente.botet" <vicente.botet@wanadoo.fr> To: <boost@lists.boost.org> Sent: Sunday, November 14, 2010 11:52 AM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono LibraryStarts TOMORROW
----- Original Message ----- From: "joel falcou" <joel.falcou@lri.fr> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 9:25 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
On 11/11/10 20:40, John Bytheway wrote:
Right; I see that. My point was that the implementation of getticks itself has lots of platform-specific code in it, and something equivalent might be usefully added to Boost (but probably not in Boost.Chrono).
Can't boost.Chrono concept wraps aroudn the concept of cycles and providde a wrapper around rdtsc adn its ilk ? Or is it more a work for StopWatch ?
Sorry for the late response, I missed this post.
As I said in another post it is very simple for the user to define a clock based on whatever cycle count the user can provide:
template <long long speed> struct cycle_count_clock { typedef typename boost::ratio_multiply<boost::ratio<speed>, boost::mega>::type frequency; // Mhz typedef typename boost::ratio_divide<boost::ratio<1>, frequency>::type period; typedef long long rep; typedef boost::chrono::duration<rep, period> duration; typedef boost::chrono::time_point<cycle_count_clock> time_point; static time_point now() { // return exact cycle count return time_point(duration(provided_by_the_user::getticks())); // access to clock cycle count } };
Boost.Chrono could provide sych a class, but the getticks() function should be provided by the user. If someone is able to provide a portable getticks() function (I'm myself unable to do it) and include it in Boost, Boost.Chrono will definitely use it to provide this cycle_count_clock.
StopWatch is at high level and builds on top of Boost.Chrono.
----- Original Message ----- From: "John Bytheway" <jbytheway+boost@gmail.com> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 8:40 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
On 11/11/10 09:54, vicente.botet wrote:
For POSIX process/thread times, I note the existence of clock_gettime(CLOCK_PROCESS_CPUTIME_ID) and clock_gettime(CLOCK_THREAD_CPUTIME_ID) which are possibly candidates for the process / thread timings. At the same time, their man page says they're unreliable on SMP systems, so probably not useful.
Oh I forget to say that for thread_clock I use clock_gettime also with the id obtained with pthread_getcpuclockid.
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.
Without the processor speed the getticks() function is not of utility for the Chrono framework. If the user know the processor speed in Mhz at compile-time s/he can define a cycle_count_clock in a simple way as follows:
Right; I see that. My point was that the implementation of getticks itself has lots of platform-specific code in it, and something equivalent might be usefully added to Boost (but probably not in Boost.Chrono).
Yes this will be very useful. No, I could not include this kind in function in Boost.Chrono, but I encourage other to provide it :) Vicente
----- Original Message ----- From: "John Bytheway" <jbytheway+boost@gmail.com> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 8:40 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
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.
Without the processor speed the getticks() function is not of utility for the Chrono framework. If the user know the processor speed in Mhz at compile-time s/he can define a cycle_count_clock in a simple way as follows:
Right; I see that. My point was that the implementation of getticks itself has lots of platform-specific code in it, and something equivalent might be usefully added to Boost (but probably not in Boost.Chrono).
I was thinking about this and if we had a cycle_count_clock class it would be easier for the user to express the time in ticks, so no transformation will be necessary when operating on them. Do you see any problems to a ticks duration class, that would be able to be converteed as the other durations? Best, Vicente
On 15/11/10 22:53, vicente.botet wrote:
Without the processor speed the getticks() function is not of utility for the Chrono framework. If the user know the processor speed in Mhz at compile-time s/he can define a cycle_count_clock in a simple way as follows:
Right; I see that. My point was that the implementation of getticks itself has lots of platform-specific code in it, and something equivalent might be usefully added to Boost (but probably not in Boost.Chrono).
I was thinking about this and if we had a cycle_count_clock class it would be easier for the user to express the time in ticks, so no transformation will be necessary when operating on them. Do you see any problems to a ticks duration class, that would be able to be converteed as the other durations?
It sounds reasonable, but a better understanding than mine of the implementation details of getticks would be required to know for sure. In particular, the one in libfftw has no provision for conversion to 'real' time units like seconds, and at present I have no reason to believe that this is possible in general; I don't know how varied the functions used are. John
----- Original Message ----- From: "John Bytheway" <jbytheway+boost@gmail.com> To: <boost@lists.boost.org> Sent: Tuesday, November 09, 2010 11:38 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
Detailed documentation comments follow. They refer to the HTML docs. They overlap with some other reviewers' comments, and some I've given in reference to certain pages also apply to subsequent pages. Where I have asked a question I may be implying that you should not only answer me but also put the answer in the docs.
Overview: - "Replaceable text that you will need to supply is in italics" is a bit self-contradictory; I suggest rather "Text for which you will need to supply a replacement is in italics".
I could change if others think it is more clear.
- "free_function" isn't monospaced, when the text implies it is.
OK.
Motivation: - The phrase "allowing the extraction of read (wall clock) time" is not clear to me. Is there such a thing as "read time"? (From later pages I guess it should be "real", I don't really like this term either, but I have no better suggestion.)
From wikipedia "In computing, wall clock time can also mean the actual time taken by a computer to complete a task. It is the sum of three terms: CPU time, I/O time, and the communication channel delay (e.g. if data are scattered on multiple machines). In contrast to CPU time, which measures only the time during which the processor is actively working on a certain task, wall time measures the total time for the process to complete. The difference between the two consists of time that passes due to programmed delays or waiting for resources to become available."
I will remove the real word as it let think that it corresponds to the time passed since the process startup and add specifically how it is decomposed.
Description: - "__process_cpuclock_" seems to be incorrectly underlined. And does the name really start with two underscores?
See the comment in the prceding mail. I will correct it.
Users' Guide: - Title is missing a space. OK.
Getting started: - s/Exceptions safety/Exception safety/ OK. - On thread safety, could you clarify how unsafe they are (when not otherwise specified). e.g. Only one thread may use boost::chrono at all, or each function may be used in only one thread at a time.
You are right this is no clear. By default we could take the more severe case, i.e. only one thread may use Boost.Chrono at a time, but this is not the case. As Boost.Chrono doesn't use mutable global variables the thread safety analysys is limited to the access to each instance variable. What I mean is that it is not thread safe to use a function that modifies the access to a variable if another can be reading or writing it. So with this intention I could add someting like that if you find clearer. I will do a review to see if there is some functions that are thread safe.
- By "not lastly" do you mean "not recently"? Yes. I will remove this anyway. - In the hello world program s/tooks/took/ OK. Tutorial: - I would remove all the apostrophes from plurals ("duration's", "time_point's"). - "__high_precisionclock_" again looks mis-underlined and I guess doesn't really start with "__". OK. - Rather than "Clock::duration d = t1 - Clock::now();" I would have expected a more common use case to be "Clock::duration d = Clock::now() - t1;" (as in the tutorial). Is that what was intended? NO. I will change. - There are some spaces in the code after "boost::chrono::", e.g. in "boost::chrono:: milliseconds"; were they deliberate? Sorry for the formatting issue. It seems this is related to the fact it is included in a link. I would try to see if I can manage with the space and remove the link otherwise.
- In 'with the proper spellings for hours, "minutes" and "seconds"', should 'hours' be in double-quotes also? Yes. - The third example in I/O has five lines sending output to cout, but only two lines in the sample output. How is this?
I need to update the output.
Examples: - Is the min function mentioned here part of the library, or simply something a user might write? It's not clear.
Well as part of the examples it seems clear that this is not part of the library. I could change to: "The user can define a function returning the earliest time_point as follows:" Let me know if this helps.
- The zero_default example is a bit intimidating. Might it be less so if you took advantage of Boost.Operator to define most of the operators?
I could do if others think it would improbe the doc.
- A round-up function appears here which is very similar to but not quite the same as the one in the Tutorial. Could one suffice? The roundup function in the tutorial contain a systax error. Rep and Period need to be prefixed by class. template <class ToDuration, Rep, Period> ToDuration
A part form this fact both roundup function are the same.
- Does the "tiny program that times how long until a key is struck" really work as the content suggests? I would expect line-buffered input on my machine, and thus the only key which will cause cin.get() to return would be Enter.
OK. I could replace by "Type the Enter key: ";
- s/simaulation/simulation/ OK.
Reference - C++0x: - s/constexpr don't used/constexpr isn't used/ OK. - s/when appropiated/where appropriate/ OK. - You say constexpr isn't used, but I see e.g. BOOST_CHRONO_CONSTEXPR macro being used. Will this automagically expand to constexpr once Boost.Config indicates support? (I see this is mentioned again in appendix F, but it's still not clear)
This is an ongoing task. Note that the macro is a Chrono macro BOOST_CHRONO_. I woud define it to constexpr when I will be able to make some test on a specific compiler. Of course the user could define the macro and see if it works.
- s/The default vaule behavior is as BOOST_CHRONO_USES_ARRAY_ASSERT was defined/The default behavior is as if BOOST_CHRONO_USES_ARRAY_ASSERT was defined/ OK. - s/texte/text/ OK. - s!files in detail/win!files in boost/detail/win! OK. - You are sometimes capitalizing Non-Member for no apparent reason. I would try to be homogeneus and capitalize all the titles.
- It would help if the not-really-code parts of the reference (such as the "see below" in common_type) were somehow distinguished; most Boost docs use italics for this. I don't know how to add italics inside the doc when using quickdoc. I will add an explicit name.
- duration member function count says "Returns: `rep_v."; should it be "Returns: rep_"? I see at least one other stray backtick/v pair on this page. OK. - The documentation of the pre-/post- inc-/dec- rement operators is a bit broken. All the headings are for inc-, not dec-, and one of the "++"s is not consistently formatted. OK. - Functions with BOOST_CHRONO_CONSTEXPR in the duration summary are not consistently presented in the details; the non-static ones have BOOST_CHRONO_CONSTEXPR, max() has constexpr, and zero() and min() have nothing at all. OK. - I don't see the definition of "milli", "micro", etc in the reference. I guess they're defined in Boost.Ratio. I think this warrants a mention. Is it enough to mention them in the dependecies section
Boost.Ratio for ratio, milli, micro, ...
- s/classes and non-member function./classes and non-member functions./ OK - The heading above the docs for operator-(const time_point<Clock, Duration1>& lhs, const time_point<Clock, Duration2>& rhs); is wrong. OK. operator-(time_point,time_point) - If BOOST_CHRONO_HAS_CLOCK_MONOTONIC is false, can the user expect monotonic_clock to be defined? NO. - Can you say anything about time zones wrt system_clock (i.e. will it be local time or UTC, or is that undefined)? The current implementation of system_clock is related an epoch (midnight UTC of January 1, 1970), but this is not in the contract. You need to use the static function static std::time_t to_time_t(const time_point& t);
that will return a time_t type, which is based on midnight UTC of January 1, 1970. It is not the role of Boost.Chrono to manage local times.
Reference - Other clocks: - How do the process clocks handle threads and child processes? Is this defined?
process clocks give the time spent by the process, threads time shall be included in but not child process. Does, this helps?
Appendix A: History: - This is empty; should it be? Other requested to remove the history and start it if the library is approved. Appendix B: Rationale: - Should the URL of N2661 be a clickable link? It isn't. PK. - s/Current N3000 doesn't allows to copy-construct or assign ratio instances/Current N3000 doesn't allow programs to copy-construct or assign instances/ - Unmatched bracket in "([LWG 1281)" This two should be removed as they are part of the Ratio library.. I though I did it:( Why ratio needs CopyConstruction and Assignment from ratios having the same normalized form Why ratio needs the nested normalizer typedef type
Appendix C: - Also empty A good place holder for some of your requests :) Appendix D: - s/sometimes gives/sometimes give/ OK
Congratulations on reaching review!
Thanks for the detailled correction. I should did a more detailled re-reading of the last edition before review. Best, Vicente
On 10/11/10 14:07, vicente.botet wrote:
From: "John Bytheway" <jbytheway+boost@gmail.com>
Overview: - "Replaceable text that you will need to supply is in italics" is a bit self-contradictory; I suggest rather "Text for which you will need to supply a replacement is in italics".
I could change if others think it is more clear.
Motivation: - The phrase "allowing the extraction of read (wall clock) time" is not clear to me. Is there such a thing as "read time"? (From later pages I guess it should be "real", I don't really like this term either, but I have no better suggestion.)
<snip>
I will remove the real word as it let think that it corresponds to the time passed since the process startup and add specifically how it is decomposed.
For me wall-clock time is a sufficient explanation, but indeed I guess some would like more information, and I'm not sure what exactly. Whatever you think is probably fine.
- On thread safety, could you clarify how unsafe they are (when not otherwise specified). e.g. Only one thread may use boost::chrono at all, or each function may be used in only one thread at a time.
You are right this is no clear. By default we could take the more severe case, i.e. only one thread may use Boost.Chrono at a time, but this is not the case. As Boost.Chrono doesn't use mutable global variables the thread safety analysys is limited to the access to each instance variable. What I mean is that it is not thread safe to use a function that modifies the access to a variable if another can be reading or writing it.
So with this intention I could add someting like that if you find clearer.
Yes, I think that would help.
I will do a review to see if there is some functions that are thread safe.
It's perhaps a bit more difficult: you want to find functions which can be safely guaranteed to be thread-safe, which might be harder.
- There are some spaces in the code after "boost::chrono::", e.g. in "boost::chrono:: milliseconds"; were they deliberate? Sorry for the formatting issue. It seems this is related to the fact it is included in a link. I would try to see if I can manage with the space and remove the link otherwise.
If it's a choice between one or the other then please keep the link, it's more useful than looking nice.
Examples: - Is the min function mentioned here part of the library, or simply something a user might write? It's not clear.
Well as part of the examples it seems clear that this is not part of the library. I could change to:
"The user can define a function returning the earliest time_point as follows:"
Let me know if this helps.
Perhaps you're right that it should be obvious, but indeed I think that helps.
- The zero_default example is a bit intimidating. Might it be less so if you took advantage of Boost.Operator to define most of the operators?
I could do if others think it would improbe the doc.
I doubt anyone else will read this far :). It's not a big issue.
- A round-up function appears here which is very similar to but not quite the same as the one in the Tutorial. Could one suffice? The roundup function in the tutorial contain a systax error. Rep and Period need to be prefixed by class. template <class ToDuration, Rep, Period> ToDuration
A part form this fact both roundup function are the same.
My point is: since they're the same is there a way to not repeat yourself without making the docs any more confusing? It would be nice, but it's not vital.
- You say constexpr isn't used, but I see e.g. BOOST_CHRONO_CONSTEXPR macro being used. Will this automagically expand to constexpr once Boost.Config indicates support? (I see this is mentioned again in appendix F, but it's still not clear)
This is an ongoing task. Note that the macro is a Chrono macro BOOST_CHRONO_. I woud define it to constexpr when I will be able to make some test on a specific compiler. Of course the user could define the macro and see if it works.
Ah, if the user can define it then I think that's worth mentioning.
- You are sometimes capitalizing Non-Member for no apparent reason. I would try to be homogeneus and capitalize all the titles.
Ah, I think I was getting confused; I didn't appreciate that they were titles. That's a good reason for capitalization. But indeed, consistency would be good.
- It would help if the not-really-code parts of the reference (such as the "see below" in common_type) were somehow distinguished; most Boost docs use italics for this. I don't know how to add italics inside the doc when using quickdoc. I will add an explicit name.
Well, as I said, other Boos libraries manage it (though I don't know whether they use quickdoc). If it's hard, don't worry.
- I don't see the definition of "milli", "micro", etc in the reference. I guess they're defined in Boost.Ratio. I think this warrants a mention. Is it enough to mention them in the dependecies section
Boost.Ratio for ratio, milli, micro, ...
Yes, that would probably suffice.
- If BOOST_CHRONO_HAS_CLOCK_MONOTONIC is false, can the user expect monotonic_clock to be defined? NO.
Oh, on closer inspection I see that this fact is evidenced by the documentation of monotonic_clock. Never mind.
- Can you say anything about time zones wrt system_clock (i.e. will it be local time or UTC, or is that undefined)? The current implementation of system_clock is related an epoch (midnight UTC of January 1, 1970), but this is not in the contract. You need to use the static function static std::time_t to_time_t(const time_point& t);
that will return a time_t type, which is based on midnight UTC of January 1, 1970.
If you're adding more details about the clocks, this warrants a mention.
It is not the role of Boost.Chrono to manage local times.
Absolutely.
Reference - Other clocks: - How do the process clocks handle threads and child processes? Is this defined?
process clocks give the time spent by the process, threads time shall be included in but not child process. Does, this helps?
Yes, I think that's worth mentioning.
Appendix C: - Also empty A good place holder for some of your requests :)
:)
Congratulations on reaching review!
Thanks for the detailled correction. I should did a more detailled re-reading of the last edition before review.
Not to worry; I know how hard it is to proofread ones own writing... John
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Anthony Williams Sent: Friday, November 05, 2010 5:26 PM To: boost@lists.boost.org Subject: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
Hi everyone,
The review of Vicente Botet's Chrono library starts TOMORROW (November 6th) and lasts until November 15th unless an extension occurs.
I've started to play with this after starting to read the (nice) docs. But I've come to a sudden halt on the "Hello World" :-( Running it without has produced the (expected) warning that I haven't built the library yet) LINK : fatal error LNK1104: cannot open file 'libboost_chrono-vc100-mt-gd-1_45.lib' Assuming I could just try it out as header only first, (before I begin swearing at bjam ;-) I've added #define BOOST_CHRONO_INLINED // run_timer_example.cpp ---------------------------------------------------// #define BOOST_CHRONO_INLINED #include <boost/chrono/process_times.hpp> #include <cmath> ... But got a surprising list of errors: C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(142): error C2008: '$' : unexpected in macro definition C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(294): error C2008: '$' : unexpected in macro definition C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(295): warning C4005: '__' : macro redefinition What am I doing wrong? Paul PS Windows 7, VS 2010, debug, console app --- Paul A. Bristow, Prizet Farmhouse, Kendal LA8 8AB UK +44 1539 561830 07714330204 pbristow@hetp.u-net.com
----- Original Message ----- From: "Paul A. Bristow" <pbristow@hetp.u-net.com> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 4:37 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.ChronoLibrary Starts TOMORROW
I've started to play with this after starting to read the (nice) docs.
But I've come to a sudden halt on the "Hello World" :-(
Running it without has produced the (expected) warning that I haven't built the library yet)
LINK : fatal error LNK1104: cannot open file 'libboost_chrono-vc100-mt-gd-1_45.lib'
Assuming I could just try it out as header only first, (before I begin swearing at bjam ;-)
I've added
#define BOOST_CHRONO_INLINED
// run_timer_example.cpp ---------------------------------------------------//
#define BOOST_CHRONO_INLINED
#include <boost/chrono/process_times.hpp> #include <cmath> ...
But got a surprising list of errors:
C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(142): error C2008: '$' : unexpected in macro definition C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(294): error C2008: '$' : unexpected in macro definition C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(295): warning C4005: '__' : macro redefinition
What am I doing wrong?
Hi, currently Boost.Chrono in a header only library if Boosy.System it is also. You need have you defined BOOST_SYSTEM_INLINED? When I remove the BOOST_SYSTEM_INLINED I get the expected error: run_timer_example.obj : error LNK2019: unresolved external symbol "class boost::system::error_category const & __cdecl boost::system::system_category(void)" (?system_category@system@boost@@YAABVerror_category@12@XZ) referenced in function "public: __thiscall boost::system::error_code::error_code(void)" (??0error_code@system@boost@@QAE@XZ) On cygwin I get the following $ g++ -I /boost/v44 ../example/run_timer_example.cpp -D BOOST_CHRONO_INLINED -D BOOST_SYSTEM_INLINED Vicente@viboes1 /boost/v44/libs/chrono/test $ ./a.exe real 0.015s, cpu 0.000s (0.0%), user 0.000s, system 0.000s Could you sent the command line you used? Best, Vicente
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of vicente.botet Sent: Thursday, November 11, 2010 4:18 PM To: boost@lists.boost.org Subject: Re: [boost] [Review] Formal Review of Proposed Boost.ChronoLibrary Starts TOMORROW
----- Original Message ----- From: "Paul A. Bristow" <pbristow@hetp.u-net.com> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 4:37 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.ChronoLibrary Starts TOMORROW
I've started to play with this after starting to read the (nice) docs.
But I've come to a sudden halt on the "Hello World" :-(
Running it without has produced the (expected) warning that I haven't built the library yet)
LINK : fatal error LNK1104: cannot open file 'libboost_chrono-vc100-mt-gd-1_45.lib'
Assuming I could just try it out as header only first, (before I begin swearing at bjam ;-)
I've added
#define BOOST_CHRONO_INLINED
// run_timer_example.cpp ---------------------------------------------------//
#define BOOST_CHRONO_INLINED
#include <boost/chrono/process_times.hpp> #include <cmath> ...
But got a surprising list of errors:
C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(142): error C2008: '$' : unexpected in macro definition C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(294): error C2008: '$' : unexpected in macro definition C:\Program Files\Microsoft SDKs\Windows\v7.0A\include\driverspecs.h(295): warning C4005: '__' : macro redefinition
What am I doing wrong?
Hi, currently Boost.Chrono in a header only library if Boosy.System it is also. You need have you defined BOOST_SYSTEM_INLINED?
I have now... #define BOOST_SYSTEM_INLINED #define BOOST_CHRONO_INLINED #include <boost/chrono/process_times.hpp> #include <cmath> produces the same output :-( I am including boost/trunk and boost/sandbox/chrono /Zi /nologo /W3 /WX- /Od /Oy- /D "WIN32" /D "_DEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /GF- /Gm /EHsc /RTC1 /GS /fp:precise /Za /Zc:wchar_t /Zc:forScope /Fp"Debug\chrono_runtime.pch" /Fa"Debug\" /Fo"Debug\" /Fd"Debug\vc100.pdb" /Gd /analyze- /errorReport:queue Perhaps I need to build the library? Hopefully it won't hurt too much ;-) Paul --- Paul A. Bristow, Prizet Farmhouse, Kendal LA8 8AB UK +44 1539 561830 07714330204 pbristow@hetp.u-net.com
----- Original Message ----- From: "Paul A. Bristow" <pbristow@hetp.u-net.com> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 5:57 PM Subject: Re: [boost] [Review] Formal Review ofProposed Boost.ChronoLibrary Starts TOMORROW
I have now...
#define BOOST_SYSTEM_INLINED #define BOOST_CHRONO_INLINED
#include <boost/chrono/process_times.hpp> #include <cmath>
produces the same output :-(
I am including boost/trunk and boost/sandbox/chrono
/Zi /nologo /W3 /WX- /Od /Oy- /D "WIN32" /D "_DEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /GF- /Gm /EHsc /RTC1 /GS /fp:precise /Za /Zc:wchar_t /Zc:forScope /Fp"Debug\chrono_runtime.pch" /Fa"Debug\" /Fo"Debug\" /Fd"Debug\vc100.pdb" /Gd /analyze- /errorReport:queue
Perhaps I need to build the library? Hopefully it won't hurt too much ;-)
Have you replaced the Boost.System directory? Vicente
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of vicente.botet Sent: Thursday, November 11, 2010 5:25 PM To: boost@lists.boost.org Subject: Re: [boost] [Review] Formal Review ofProposed Boost.ChronoLibrary Starts TOMORROW
I have now...
#define BOOST_SYSTEM_INLINED #define BOOST_CHRONO_INLINED
#include <boost/chrono/process_times.hpp> #include <cmath>
produces the same output :-(
I am including boost/trunk and boost/sandbox/chrono
/Zi /nologo /W3 /WX- /Od /Oy- /D "WIN32" /D "_DEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /GF- /Gm /EHsc /RTC1 /GS /fp:precise /Za /Zc:wchar_t /Zc:forScope /Fp"Debug\chrono_runtime.pch" /Fa"Debug\" /Fo"Debug\" /Fd"Debug\vc100.pdb" /Gd /analyze- /errorReport:queue
Perhaps I need to build the library? Hopefully it won't hurt too much ;-)
Have you replaced the Boost.System directory?
I've just updated the boost/sandbox/chrono/ directory. It has a system folder Do I have to do something else? (I assumed - the mother of all foul-ups - that putting sandbox first in include order would find any new version). Thanks Paul
----- Original Message ----- From: "Paul A. Bristow" <pbristow@hetp.u-net.com> To: <boost@lists.boost.org> Sent: Thursday, November 11, 2010 7:00 PM Subject: Re: [boost] [Review] FormalReview ofProposed Boost.ChronoLibrary Starts TOMORROW
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of vicente.botet Sent: Thursday, November 11, 2010 5:25 PM To: boost@lists.boost.org Subject: Re: [boost] [Review] Formal Review ofProposed Boost.ChronoLibrary Starts TOMORROW
I have now...
#define BOOST_SYSTEM_INLINED #define BOOST_CHRONO_INLINED
#include <boost/chrono/process_times.hpp> #include <cmath>
produces the same output :-(
I am including boost/trunk and boost/sandbox/chrono
/Zi /nologo /W3 /WX- /Od /Oy- /D "WIN32" /D "_DEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /GF- /Gm /EHsc /RTC1 /GS /fp:precise /Za /Zc:wchar_t /Zc:forScope /Fp"Debug\chrono_runtime.pch" /Fa"Debug\" /Fo"Debug\" /Fd"Debug\vc100.pdb" /Gd /analyze- /errorReport:queue
Perhaps I need to build the library? Hopefully it won't hurt too much ;-)
Have you replaced the Boost.System directory?
I've just updated the boost/sandbox/chrono/ directory.
It has a system folder
Do I have to do something else?
Not that I know.
(I assumed - the mother of all foul-ups - that putting sandbox first in include order would find any new version).
This seems a good assumption. I have not tested with this schema recently, I use to test on a single boost tree. Sorry, but I'm out of ideas for the moment. Best, Vicente
Hi, I have tested the constexpr with gcc-4.6. I would like to commit these changes on the sandbox, but I wanted to ask the ML before in case this could disturb the review. As the reference of the review is the file on the Vault, I think that committing these changes should not disturnb the review. Anthony, could I commit? I have also make some modifications to the documentation following the comments of the reveiwers (even if not done completly yet). Anthony, could I commit the documentation changes? Best regards, _____________________ Vicente Juan Botet Escribá http://viboes.blogspot.com/
"vicente.botet" <vicente.botet@wanadoo.fr> writes:
I have tested the constexpr with gcc-4.6. I would like to commit these changes on the sandbox, but I wanted to ask the ML before in case this could disturb the review. As the reference of the review is the file on the Vault, I think that committing these changes should not disturnb the review. Anthony, could I commit?
I have also make some modifications to the documentation following the comments of the reveiwers (even if not done completly yet). Anthony, could I commit the documentation changes?
Please commit all your changes, if you haven't already. Anthony -- Author of C++ Concurrency in Action http://www.stdthread.co.uk/book/ just::thread C++0x thread library http://www.stdthread.co.uk Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK. Company No. 5478976
On 11/5/2010 11:25 AM, Anthony Williams wrote:
- What is your evaluation of the design?
I think the design is well thought out. It cleanly separates out the basic operations for capturing, constructing, and comparing points in time from the higher-level operations of relating times to things like the Gregorian calendar. At the same time, it provides a good platform upon which these higher-level operations can be built.
- What is your evaluation of the implementation?
I have not delved into all parts of the implementation, but what I have looked at seems sound.
- What is your evaluation of the documentation?
The documentation is good. It gives a clear explanations of the basic components of the design (durations, time_points, and clocks). It also does a good job of explaining the more subtle issues of converting between different types of durations and when exact conversions are guaranteed vs. when approximation will be performed. There are small typographical or grammatical errors here and there in the documentation, but I believe others have already covered these fairly comprehensively.
- What is your evaluation of the potential usefulness of the library?
Very useful. Chrono provides the potential for an extensible and efficient way to standardize use of time across the many Boost libraries that traffic in timing, timers, and waiting (Boost.Thread, Boost.Asio, etc).
- Did you try to use the library? With what compiler? Did you have any problems?
Yes, with VC9 SP1 on Win7. I ran into some minor issues with pieces of the ratio library triggering internal compiler errors on VC9, but I was able to resolve these. I also noticed that the jam files for the regression tests reference test files that did not seem to be included in the distribution in the sandbox. Both of these should be easily fixed. I also did some testing with VC9 SP1 cross-compiling for Windows CE 5.0. I ran into some minor issues (as expected) mainly related to needing the combination of GetSystemTime and SystemTimeToFileTime calls instead of GetSystemTimeAsFileTime (which the WinCE libraries sadly do not supply). Other than this, it functioned well.
- How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study?
I wouldn't say an in-depth study, but some fairly involved work. We had integrated some of the earlier versions of Boost.Chrono into our local Boost tree and had made adaptations to Boost.Thread and Boost.Asio to make use of the Chrono functionality. The Chrono-based Asio was used in some applications that do automated data collection from web pages, and Chrono-based Thread and Chrono itself were used within Win32 and WinCE applications for automated control systems. Some of the basic Chrono components were used for performance timing in tuning of application code.
- Are you knowledgeable about the problem domain?
Somewhat, but certainly not an expert.
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
Yes, I think Chrono should be accepted into Boost. Thanks, Vincente, for all your hard work in crafting this into a formal library submission, seeing through the restructuring to properly partition all the components, and filling in the documentation and testing. -Dave Deakins
----- Original Message ----- From: "David Deakins" <ddeakins@veeco.com> To: <boost@lists.boost.org> Sent: Tuesday, November 16, 2010 8:31 AM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
- Did you try to use the library? With what compiler? Did you have any problems?
Yes, with VC9 SP1 on Win7. I ran into some minor issues with pieces of the ratio library triggering internal compiler errors on VC9, but I was able to resolve these. I also noticed that the jam files for the regression tests reference test files that did not seem to be included in the distribution in the sandbox. Both of these should be easily fixed.
Could you send me the errors and how you solved them? Could you give me the name of the files. Maybe I missed the commit of these files?
I also did some testing with VC9 SP1 cross-compiling for Windows CE 5.0. I ran into some minor issues (as expected) mainly related to needing the combination of GetSystemTime and SystemTimeToFileTime calls instead of GetSystemTimeAsFileTime (which the WinCE libraries sadly do not supply). Other than this, it functioned well.
I have no access to VC9 SP1 for Windows CE 5.0. Could you send me a the needed modifications?
- How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study?
I wouldn't say an in-depth study, but some fairly involved work. We had integrated some of the earlier versions of Boost.Chrono into our local Boost tree and had made adaptations to Boost.Thread and Boost.Asio to make use of the Chrono functionality. The Chrono-based Asio was used in some applications that do automated data collection from web pages, and Chrono-based Thread and Chrono itself were used within Win32 and WinCE applications for automated control systems. Some of the basic Chrono components were used for performance timing in tuning of application code.
Glad to hear you find it useful. I hope we will have the oportunity to update the Boost libraries that can make use of Boost.Chrono soon. Let me know if Boost.Stopwatch respond to your performance timing needs.
- Do you think the library should be accepted as a Boost library?
Yes, I think Chrono should be accepted into Boost.
Thanks, Vincente, for all your hard work in crafting this into a formal library submission, seeing through the restructuring to properly partition all the components, and filling in the documentation and testing.
You are welcome. I started this work one year ago after Beman told me he was not interested in submiting it to Boost because some compilers started to provide Chrono as part of its C++0x library. I considered that we needed a C++03 compatible version and I accepted to take the relay. At the begining I though that the library was almost ready, and people started to request me more a more things, so I added this and that until the split of Boost Chrono in some additions to TypeTraits and 3 libraries: Ratio, Chrono and Stopwatch. I have learn a lot on this walk and I want to thank every people that help me to improve these library. Thanks for your review, Vicente
On 11/16/2010 1:29 PM, vicente.botet wrote:
From: "David Deakins"<ddeakins@veeco.com>
- Did you try to use the library? With what compiler? Did you have any problems?
Yes, with VC9 SP1 on Win7. I ran into some minor issues with pieces of the ratio library triggering internal compiler errors on VC9, but I was able to resolve these. I also noticed that the jam files for the regression tests reference test files that did not seem to be included in the distribution in the sandbox. Both of these should be easily fixed.
Could you send me the errors and how you solved them? Could you give me the name of the files. Maybe I missed the commit of these files?
Here is an example of the kind of compiler errors that I was seeing on VC9 SP1 when I ran the Chrono and Ratio regression tests: ..\libs\ratio\test\..\example\si_physics.cpp(199) : fatal error C1001: An internal error has occurred in the compiler. (compiler file 'msc1.cpp', line 1411) To work around this problem, try simplifying or changing the program near the locations listed above. Please choose the Technical Support command on the Visual C++ Help menu, or open the Technical Support help file for more information Internal Compiler Error in c:\Program Files\Microsoft Visual Studio 9.0\VC\BIN\cl.exe. You will be prompted to send an error report to Microsoft later. Attached is the patch to boost/ratio/ratio.hpp that seems to prevent these errors. Also attached is a patch file showing the regression tests that I had to comment out in the chrono test jamfile because they didn't appear to be in the sandbox. I'll send more info about the WinCE 5 stuff in the near future (those adjustments were a little bit more extensive). Thanks, -Dave
Heads up: The C++ committee renamed std::chrono::monotonic_clock to std::chrono::steady_clock. The is_monotonic bool was also renamed to is_steady. This will be documented in N3191 some time next week. -Howard
Message du 20/11/10 19:56 De : "Howard Hinnant" A : boost@lists.boost.org Copie à : Objet : Re: [boost] [Review] Formal Review of Proposed Boost.Chrono LibraryStarts TOMORROW
Heads up: The C++ committee renamed std::chrono::monotonic_clock to std::chrono::steady_clock. The is_monotonic bool was also renamed to is_steady. This will be documented in N3191 some time next week.
Thanks for the info. Vicente
participants (9)
-
Anthony Williams -
David Deakins -
Howard Hinnant -
joel falcou -
John Bytheway -
Paul A. Bristow -
Roland Bock -
Vicente BOTET -
vicente.botet