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: John Bytheway (jbytheway+boost_at_[hidden])
Date: 2010-11-09 17:38:23

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