Re: [boost] [locale] Formal review of Boost.Locale library -- final days

Subject: Re: [boost] [locale] Formal review of Boost.Locale library -- final days
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-04-23 15:07:47

> From: Gevorg Voskanyan <v_gevorg_at_[hidden]>

> Points I don't like most:
>
> -  The boundary interface isn't attractive (in my opinion, shared by some other 
>
>
> reviewers)

As it was common request that was shared by several reviewers
I'll take a look on it. And I'll also take a look on
regex like interface that may actually be relevant for
this case.

> - Operator overloading is sometimes used in unintuitive ways.  For example 
> operator/ used to get the value for a specific period  type
> 

I'll also update a little date_time the interface to be consistent
with other boost libraries, for example to get

   year(t)
   t=year(1990);

In addition or instead of 

   t = year * 1990
   t / year

> [snip]
>
> I noticed lack of exception safety  in some places and places where RAII 
> should have been used. The  implementation might benefit from a careful 
> examination intended to find and  fix such kind of problems.

I've seen several places that were pointed by reviewers and I had
seem some problems with copy constructors in the code so I'll
review them.

> I also think that 
> dynamic memory management is  used too freely - in many cases their usage is 
> unavoidable but in some cases  they are. Having said that I appreciate that 
>some 
>
> efforts have already been  put in that direction: I noticed several places 
>where 
>
> fixed-size buffers  were used if data is small enough and dynamic allocation if 
>
>
> it's  larger.
> 
> More details at  http://permalink.gmane.org/gmane.comp.lib.boost.devel/218352
> 

Noted.

> > - What  is your evaluation of the  documentation?
> 
> The documentation  structure is good. It enables a new user to get started with 
>
>
> the library  quickly. The well-organized references help to find documentation 

> for a  specific facility in almost no time. I would however like to see more 
> links  within the text of the pages. There are many spelling and grammar issues 
>
>
> in  the documentation. Many of them have been already addressed to some extent 

>by 
>
> me and other reviewers, but it feels like there are still more of them left 
> there. I agree that the documentation should be thoroughly scanned for such 
> language issues by a native English speaker.
> I also think there should be  more examples.
> 
> More details at  http://permalink.gmane.org/gmane.comp.lib.boost.devel/218117 
>and 
>
> http://permalink.gmane.org/gmane.comp.lib.boost.devel/218343
> 

I'll do my best, hopefully after third language review you did it would
be much better.

> > -  What is your evaluation of the potential usefulness of the   library?
> 
> Very useful.
> 
> > - Did you try to use the library? With  what compiler? Did you have  any
> >   problems?
> 
> I have  played with the library using number/ordinal formatting, date 
>formatting, 
>
> currency formatting, spellout, case conversion and boundary analysis  features. 
>
>I 
>
> have not used encoding conversion, normalization,  timezone/calendar, (message) 
>
>
> formatting and collation features. I have used  ICU backend only.
> 
> Environment:
> FreeBSD 8.0
> g++ 4.5.0 in C++03  mode
> ICU 3.8.1 system-wide installation
> 
> What I used was intuitive and  easy, except for boundary analysis as noted 
>above.
> 
> I had some incorrect  results most of which I believe are ICU issues.
> Some which I'm not so sure  about though:
> - output of as::local_time was wrong by 2 hours; as::gmt was  correct. 
>Moreover, 
>
> as::date printed the yesterday's date when the local time  was 2:24 AM !
> /bin/date shows correct local time, so I believe my system time  zone settings 

> are fine.

This is problem with older ICU versions. ICU had included
several bugs in the local time zone detection. I assume that
the test test_icu_of_os_timezone fails.

I've seen things things on FreeBSD:

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

(Even it is not tested under Boost namespace it still catches
the problems)

What you need is latest ICU. It had several fixes
of timezone detection in 4.6 and this version works
fine and passes all the tests.

So I recommend you to use it with ICU 4.6 on FreeBSD.

There is a some workaround I implemented for Linux for
other problem of detecting timezone as "localtime" but
it seems that on FreeBSD it is other issue as it
has little bit different timezone handling.

> - "Note: not all locales provide rules for  spelling numbers. In such a case 
>the 
>
> number would be displayed in decimal  format." This was not the case:        
> std::cout  << as::spellout << 117 << std::endl; printed "one hundred and 
> seventeen" under hy_AM.UTF-8 locale.

I'll take a look on this, it may be either CLDR issue or ICU mis-detecting
hy_AM locale.

> 
> There were loads of "comma at  end of enumerator list" warnings.
> 
> > - How much effort did you put into  your evaluation? A  glance? A quick
> >   reading? In-depth  study?
> 
> Closer to in-depth study with more than 25 hours  invested.
> 
> > - Are you knowledgeable  about the problem  domain?
> 
> I am well aware of localization problems and approaches that can  be used to 
> tackle them. I have developed in-house solutions to address some  localization 

> needs on as-necessary basis.
> 
> > And finally, every  review should answer this  question:
> > 
> > - Do you think the  library should be accepted as a Boost  library?
> 
> Yes.

Thank you very much for the review.

  Artyom