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.)
> 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
Yes, I think that would help.
> I will do a review to see if there is some functions that are thread
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
> Let me know if this helps.
Perhaps you're right that it should be obvious, but indeed I think that
>> - 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
> 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
> 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?
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.
>> 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...
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk