On 06/04/11 22:29, Chad Nelson wrote:
The general review checklist:
- What is your evaluation of the design?
Overall the design seems well thought out. The following stand out as good points: - The availability of various backends reassures me that the design is appropriately general and flexible. - The option to treat std::string as UTF-8 is welcome; I am fed up with e.g. Glib which insists on assuming std::string is in the default locale. - I get the impression that Artyom has been using the library extensively and it is better for that experience. Only one worry presented itself to me as I read the docs. Consider the following example: date_time some_point = period::year * 1995 + period::january + period::day*1; It looks like a period can be implicitly cast to a date_time. This seems dangerous. I feel the start of 1995 should be obtained more explicitly through the calendar.
- What is your evaluation of the implementation?
Did not look.
- What is your evaluation of the documentation?
The prose of the documentation is very good, and the tutorials do a good job of introducing the various aspects. I did not look at the reference in detail, but I am more concerned that it does not offer the necessary level of detail. Consider for example the documentation for normalize (<http://cppcms.sourceforge.net/boost_locale/html/group__convert.html#ga11672e3cc3ed7eecf1dd07060265aab2>). I went to the reference for this because the tutorial (<http://cppcms.sourceforge.net/boost_locale/html/conversions.html>) did not explain. I see that it performs Unicode normalization, but there is no hint as to what value I may pass as the 'n' parameter. Where are these documented? This is a rhetorical question; I know the answer is "at the top of the page", but a link would be useful. There are many terms used in the documentation which may not be familiar to all readers. e.g. what it means to "fold case" and what the definitions of the various normalization forms, collation levels, etc. are. Some terms are covered in the glossary, but not all and only briefly. There are some links to Wikipedia articles, but not many. I suggest that links to other resources such as more Wikipedia articles, Unicode Consortium documents, etc., especially in the glossary or in a new "references" appendix but also wherever else appropriate (perhaps linking via a list in the appendix). I second the request for previous/next links. It's hard to browse without them. There are many code examples, most of which involve writing something to stdout, but few of them include examples of possible output. It would be extremely helpful to add example output (preferably more than one example where the output can vary by locale) to more of the examples. Assorted minor issues: In "Locale Generation": - You don't mention what character delimits multiple "variant" options in a locale identifier. Is this even possible? This suggests so, but later the boost::local::info::variant function implies that it is not possible. In "Conversions" - You should mention at least briefly what fold_case and normalize do. In "Messages formatting": - There are four "The source text is not copied." with different emphasis. I think the second and fourth should read "The source text is copied.". In that same paragraph the formatting is wrong near "Plural form translation: Translate a message"; it's missing a newline and wrongly indented. - The example of multiple message domains defines some hypothetical domains, but doesn't use them in the example; it would be clearer to connect the two. - In the full list of xgettext parameters most but not all of the function names given are links to their respective docs. Could they all be? In "Localized Text Formatting": - You do not explain how to include a literal '{' character in the string. - The syntax grammar seems wrong. 'key' seems to be being defined to be just one character, I guess there should be a '*'? - "date, time , datetime" has a stray space after "time". - It says "See as::ftime manipulator.", but the "as::ftime" is not a link. Could it be? In "Working with dates, times, timezones and calendars.": - Just how general is the calendar format here? In "Using Localization Backends": - You discuss on what platforms the standard library backend is useful. Where you say "on Linux with GCC or Intel compilers", would it be better to say "when using the GCC standard library, for example with GCC, the Intel compiler" since I guess e.g. clang (if used with the gcc std lib) would work too? - A comment in an example has stray Doxygen markup: "select \c std backend as default", and again later: "since it is not supported by \c std". Misc: - There was some earlier discussion of whether Boost.Locale should provide the ability to load gettext catalogs from other sources than the filesystem. Was this resolved? It might warrant a mention in the rationale.
- What is your evaluation of the potential usefulness of the library?
Very useful. I have in the past written programs for which I would have liked to include localization support, but could find no existing library which presented an interface nice enough that I felt I could use it. Boost.Locale's interface looks good enough that I might.
- Did you try to use the library? With what compiler? Did you have any problems?
I did not try.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A reading of the tutorials only, not the reference.
- Are you knowledgeable about the problem domain?
No. As a native English speaker I live in blissful ignorance of most of the difficulties :). I hope some users of diverse languages and cultures get the chance to try the library during the review.
Please explicitly state in your review whether the library should be accepted.
Yes it should. I send this review now since I suspect I will not get around to looking at the library in more detail or try it myself. If I do manage that I will be sure to report back further. For now, thanks very much to Artyom and good luck with the review. John Bytheway