Re: [boost] [Boost.Locale] For Preliminary Review of Boost Community (with Full documentation)

Subject: Re: [boost] [Boost.Locale] For Preliminary Review of Boost Community (with Full documentation)
From: Markus Raab (usenet_at_[hidden])
Date: 2009-11-16 16:18:27

Hello List!

Artyom wrote:
> I'm waiting for your inputs

First I have to congratulate the author that he tries to fill a dark hole of
current c++. I am afraid it is impossible to make such a library optimal
for everyone. But there are many parts which are really great:
- there are already many features
- case conversion, case folding and normalization is done nice
- as::spellout and so on is convenient
- message formatting is like stated very powerful
- std::string is supported (no new string type introduced)
- but others are supported to
- very good integration in iostreams and facets

The overall design fits very well into C++ and boost.
But for boundary analysis I have other expections:
I would rather expect an iterator similar to boost::tokenizer
http://www.boost.org/doc/libs/1_40_0/libs/tokenizer/index.html
which lets me iterate over characters, words and sentences.
The design of your boundary analysis seem to be very complicated and because
of the manual use of offsets very error prone.

There is a part where I see intersection to existing boost libraries:
- date formatting, especially the ftime functionality are already supported
by boost::date_time, see:
http://www.boost.org/doc/libs/1_40_0/doc/html/date_time/date_time_io.html#date_time.format_flags
- time zones, also already in boost::date_time
however I find your way better that you use std::set<std::string> and not
std::vector<std::string> like in the Time Zone Datebase of boost::date:
http://www.boost.org/doc/libs/1_40_0/doc/html/date_time/local_time.html#date_time.local_time.tz_database

On the other hand there are parts inside boost which should use
boost::locale:
- boost::regex (seems like it is currently using libicu directly?)
- boost::format

Some remarks about the doxygen documentation:
http://cppcms.sourceforge.net/boost_locale/docs/doxy/html/

- It should read #include <boost/locale/generator.hpp>
instead of #include <generator.hpp> (and all the others).

- impl() should be private and should not be documented. "Do not use this"
is not very useful.

- Documentation missing for operator== in timezone: Does it compare object
identity or id?

Now I will discuss the problems in the order of the tutorial:
http://cppcms.sourceforge.net/boost_locale/docs/

- I did not find a list of all available facets. How can I get back to the
defaults (all facets activated) after some are turned off?

== Collation ==

- There should be some explanation of std::use_facet or facet in general
because it is a very seldom used feature of the C++ standard.
Thats because without something like boost.locale it is barely useable.

- loc was used in
use_facet<collator<wchar_t> >(loc).compare(collator_base::secondary,a,b);
but not defined before (inside Collation chapter). Better you use locale().
The same problem with some_locale and some_level.
Give working examples whenever it is easily possible.

- You wrote the comment:
// Now strings uses default system locale for string comparison
Does it means that it uses the default system locale ("C"), the locale when
you call gen(""); or the default which was set before with
std::locale::global()?

I would suggest some glossary, like in boost::filesystem:
http://www.boost.org/doc/libs/1_40_0/libs/filesystem/doc/reference.html#Definitions

== Number, Time and Currency ==

- What does "as" mean? Is this just meant as the english word as?

- Negative Numbers as currency do not work as expected. The output of the
program with a de_AT locale is:
-¤ 14,00
but I would expect ¤ -14,00

- Parsing has the same problem, but even worse the varialbe contains a
completely wrong number afterwards!
Parsing
¤ -14,00
leads that the variable contains 11027 afterwards! The library really should
throw an exception if there are parsing errors.

- You did not mention that cout.imbue() or cin.imbue() must be called first
in the beginning of the section. Just setting the global locale (or default
locale?) is not enough.

- as::ordinal did not work.
In the locale de_AT I would expect << boost::locale::as::ordinal << 1 to
output "1.". But it just outputs "1". The rule is simple: just add a "."
for every number.

- as::spellout (as stated above) is really convenient but I am missing a
spellout for ordinal numbers (first, second, third, fourth or in german
erste, zweite, dritte, vierte)

- I missed the place where it is stated that e.g.
boost::locale::as::currency_iso needs a boost::locale::as::currency before.

- boost::locale::as::currency_iso does not work:
std::cout << boost::locale::as::currency << boost::locale::as::currency_iso
<< 1523.45 << std::endl;
¤ 1.523,45
std::cout << boost::locale::as::currency_iso << 1523.45 << std::endl;
1523.45
But it should read:
1.523,45 EUR

- Why is the type double used for time? You should use time_t or TimeType
instead. double is used in the example, but also in offset_from_gmt.

- Also for time it should be more clear that e.g. time_long is a additional
modificator to time.

- gmt seems to modificate both. time_zone also modificated both, even though
it has the prefix "time_". All others with that prefix only modificate time
formatting. Maybe you should avoid prefixing and use a namespace instead?

== Message Formatting ==

- typing mistakes:
signle -> single
boost::locale::message boost::locale::translate(const char*, const char*,
int) <- last int missing

- as::domain did not work, when using:
std::cout << boost::locale::as::domain("foo") <<
boost::locale::translate("foo") << std::endl;

it results to the compiler error:
/usr/src/locale/libs/locale/../../boost/locale/message.hpp: In
function ?std::basic_ostream<CharType, std::char_traits<_CharT> >&
boost::locale::as::det    ails::operator<<(std::basic_ostream<CharType,
std::char_traits<_CharT> >&, const boost::locale::as::details::set_domain&)
[with CharType = char]?:
/usr/src/locale/libs/locale/examples/h.cpp:25:   instantiated from here
/usr/src/locale/libs/locale/../../boost/locale/message.hpp:391: error: no
matching function for call to ?use_facet(std::locale)?

- Why is wchar_t used even though it is assigned to a string?
std::string msg = translate("Do you want to open the
file?").str<wchar_t>(some_locale)

- The datatype "message" in send_to_all is not explained? What is "ms"?

- " missing and translate misspelled in:
cout << format(tranlsate("You have 1 file in the directory",You have {1}
files in the directory",files)) % files << endl;

- gettext, ngettext are exposed into global namespace when
<boost/locale.hpp> is included!

== Code-page conversions ==

- Please give an example for code converter.

== Boundary analysis ==

- Spelling: indx should be index?

- first test example does not output any character (only line breaks)

- Like above said, shouldn't it be a iterator interface?

== Info ==

- misspelling: tranlsate

- std::locale::name (or better std::locale().name()) just returns *, even
german locals are set. I would expect that your library sets a name?

- How is info meant to be used? ~info is protected, so it seems like that
the object should not be instanciated myself?

== Multiple Locales ==

- Do you mean gen.get() in:
std::locale ar=get("ar_EG");

Why is it not cached in that case?

== general ==

- What is your evaluation of the potential usefulness of the library?
indispensable

- Did you try to use the library?  With what compiler?
gcc (Debian 4.3.2-1.1) 4.3.2 ICU 3.6-2etch3

- How much effort did you put into your evaluation?
about 6 hours

- Are you knowledgeable about the problem domain?
yes

You have done good work and I am sure that it has the opportunity to get the
standard for localization efforts. Keep at it!

best regards
Markus Raab

-- 
http://www.markus-raab.org | Without a good library, most interesting
                      -o)  | tasks are hard to do in C++; but given a
Kernel 2.6.24-1-a      /\  | good library, almost any task can be made
on a x86_64           _\_v | easy.  -- Bjarne Stroustrup