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