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

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

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

> 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