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-10 15:07:13

On 10/11/10 14:07, vicente.botet wrote:
> From: "John Bytheway" <jbytheway+boost_at_[hidden]>
>> 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