Re: [boost] [locale] Formal review of Boost.Locale library -- final days
From: Gevorg Voskanyan <v_gevorg@yahoo.com>
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
Artyom wrote:
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.
OK.
- 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
That will be nicer. [snip]
- 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.
Indeed, and it's the only failing test.
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.
Ok, thanks for the info, it helps.
- "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.
Thanks :)
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
Thank you very much for Boost.Locale! Best Regards, Gevorg
participants (2)
-
Artyom -
Gevorg Voskanyan