Re: [boost] [locale] Review of the proposed Boost.Locale library

Subject: Re: [boost] [locale] Review of the proposed Boost.Locale library
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-04-18 17:07:34

> From: Matus Chochlik <chochlik_at_[hidden]>
> 
> -  Please explicitly state in your review whether the library should  be
> accepted.
> 
> Yes, it should be accepted.
> 

I'm glad to hear.


> [snip]
> 
> AFAIK the Boost Coding Guidelines require max  80
> characters per line in the source code. The sources
> of Boost.Locale do  not follow this guideline which
> makes them slightly less readable, one  needs
> to scroll a lot horizontally. Otherwise I leave the quality
> of the  implementation to be judged by more competent
> people on this  list.
> 

Small point, there are lots of code in boost
that does not follow 80 characters limit.

Also I understand the desire to put somewhere
a limit (derived from the size of a punch card
back to 60th)

But sometimes it just make it more
then less readable.

> [snip]
>
> - What is your evaluation of the documentation?
> 
> The  documentation is readable and clear to someone
> familiar with l10n/i18n  however things like collation,
> capitalization, case folding, etc. should be  better
> explained for users who are not familiar with them.
> The  previous/next page links are missing which is
> little inconvenient.

Noted.

> 
> In  the Message Formatting/Message Translation section
> there is a  typo:
> ------------------------------------------------------------------------------
>     std::cout << boost::locale::tanslate("Hello World!") <<  std::endl;
> ------------------------------------------------------------------------------
> 

Noted.


> In  the examples and code snippets in the docs I think
> that "using namespace  std;" should be removed
> and instead of
> "using namespace  boost::locale;"
> 
> there should be something like
> "namespace bl =  boost::locale;"
> 
> which would make much clearer which parts of the sample  code
> are referring to Boost.Locale's code and which to std  library;

Good point.

> 
> Also (but I might be mistaken here since I'm not a  native
> German speaker) the example in the "Conversions"
> section names a  variable 'gruben' (this variable stores
> the word "grüßen") a German would  probably
> call this variable 'grussen' since 'ß' != 'b'.
> 

Yes, you are right grussen is better.

> [snip]
> 
> - Did you try to use the  library? With what compiler? Did you have any
> problems?
> 
> Yes (I built  the library),
> - on Debian Linux (64bit) with ICU 4.6
>   - using  cmake
>     - with GCC 4.6.0 non-C++0x mode: no problems
>      - with GCC 4.6.0 C++0x mode: auto_ptr - related warnings
>   -  using bjam (Boost 1.46.1)
>     - with GCC 4.6.0 non-C++0x mode: no  problems
> - on Windows Vista Basic (32-bit) + cygwin + ICU 4.5
>   -  using bjam (Boost 1.46.1)
>     - with GCC 4.3.4: no problems
>  -  on Windows Vista (32-bit) + ICU 4.6 (bin package for Win32)
>   - using  bjam (Boost 1.46.1)
>     - with MSVC 9: the library built, but for  some reason
>       bjam didn't use ICU even with -sICU_PATH  specified,
>       and I didn't have the time to investigate  why.
> 

It may be due to fact that you used provided
ICU packages (for MSVC10 and they release only)

It is always better to build ICU by yourself.
They provide visual studio projects.

I probably need to state this more explicitly
in tutorial - how to build ICU with MSVC.


> [snip]
> 
> 
> Best regards,
> 
> Matus  Chochlik

Thank you very much for the review.
  Artyom