Re: [boost] [locale] Formal review of Boost.Locale library starts tomorrow

Subject: Re: [boost] [locale] Formal review of Boost.Locale library starts tomorrow
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-04-08 06:57:56

> 

> - I  get the impression that Artyom has been using the library
> extensively and it  is better for that experience.
>

It is actually integrated part of CppCMS project.

For example this wiki (written in C++/CppCMS)

  http://art-blog.no-ip.info/wikipp/en/page/main

Is localized using Boost.Locale and it uses collation,
date/time formatting message translation and multiple
locales in same process.

So yes, I definitely use it all the time :-). 

 
> 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.
> 

Actually date_time by default initialized to current time. So if you
set a year 1995 it would be today's date and time back into 1995.

I prefer not to use explicit cast as it makes life much easier
for users and also 

  period::year * 1995 + period::january + period::day*1

Is a periods set has its own type that are set later together.

Also small notices period::january is alias for period::month*0 and
has different type then period::month.

> 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.
> 

Strange that Doxygen didn't inserted like to norm_type automatically. I'll
add it and try to look over similar places.

> 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).
> 

Good point, I'll address it.

> I second the request for previous/next links.  It's hard  to browse
> without them.
> 

Ok, I'll see how to do this, unfortunately Doxygen does not know
to do them automatically, but at the top you always have
a link upper and you can go forward.

> 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.
> 

Ok, I'll try to address this where I can.

> 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.
> 

Actually variant support vary by backend. Unix supports
predefined set like @latin or @euro and ICU handles them
well. Windows backends does not recognize them and
ICU provides its own syntax. Like

   calendar=hebrew;collation=phonetic

I'll add these examples.

> In "Conversions"
> 
> - You should mention  at least briefly what fold_case and normalize do.

Good point.

> 
> 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.
> 

Ok, missed that you are right.

> - 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.
> 
Good point. I'll address it.

> - 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?

Actually it is something that Doxygen did for me implicitly, the point
is that u16* and u32* are hidden as they mostly not in use. I'll see if
this is possible to fix this.

> 
> In "Localized Text  Formatting":
> 
> - You do not explain how to include a literal '{' character  in the string.

Good point, forgot this, it is '{{' and '}}'
> 
> - The syntax grammar seems wrong.  'key' seems to be  being defined to be
> just one character, I guess there should be a  '*'?
> 

Yes should be +

> - "date, time , datetime" has a stray space after "time".
> 

Ok I'll fix this.

> -  It says "See as::ftime manipulator.", but the "as::ftime" is not  a
> link.  Could it be?

I'll add it.

> 
> In "Working with dates, times, timezones  and calendars.":
> 
> - Just how general is the calendar format  here?

What do you mean?

ICU supports several calendars (Hebrew, Islamic, Japanese, Chinese and some 
more)
and their support vary by ICU version.

> 
> 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?
> 

I can't tell, not tested with clang. 

But I must say that toooo many standard libraries has broken locale
support. For example GCC's libstd++ supports locales only
on Linux based on libc's (now POSIX but in earlier days non-standard
functions like newlocale/duplocale).

So I would not be surprised that clang's version of libstd++ compiled
with only C/POSIX locales in.

For my experience there are very few standard C++ libraries that do this
well. Even stlport's locales quite broken (see faults of SunStudio
with stlport in tested platforms/compilers list)

> - 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".
> 

Noted.

> 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.

Ok this is excellent question.

Few days ago I passed once again on older reviews and found out that
I forgot the request.

It is implemented and documented now in trunk but it
was fully tested only in last nightly build (as part of CppCMS)

  http://art-blog.no-ip.info/files/nightly-build-report.html

So I decided not to take a chance of breaking current release
and not included it in review version.

Basically there is an additional callback of type:

   boost::function<std::vector<char>(std::string const &file_name,std::string 
const &encoding)> 

In messages_info structure. See:

  http://cppcms.sourceforge.net/boost_locale/html/gnu__gettext_8hpp_source.html

So a custom file system support can be installed in few
lines of code (example exists in trunk as well)

> 
> > 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
> 

Thank you very much for the review and many important
inputs.

Artyom