Subject: Re: [boost] [locale] Review
From: Mathias Gaunard (mathias.gaunard_at_[hidden])
Date: 2011-04-19 11:13:24
On 18/04/2011 14:24, Artyom wrote:
> Can you please state explicitly what things are
> conditional for the library to get to mini-review.
I don't know, I just read some more of the other reviews, and no one
seems to have the same concerns as me, therefore I do not know if my
demands are justified.
> Boost.Locale is build from very practical
> point of view - primary usability. I don't
> try to solve all possible corner cases, especially
> when they are not central part of localization
> I'd rather prefer spending more time on adding
> specific missing features useful for localization
> over providing stuff like iterator based interface
> for some operations that usually executed
> on small chunks of text.
I think that for a Boost library, the API can be more important than the
>> The main reason for my decision is the
>> interface that the library provides:
>> albeit it aims at abstracting the
>> backend used to implement the
>> facilities, it exposes too much of
>> how those facilities are implemented,
>> which prevents benefiting from future
>> backends that may not have the same
>> limitations as the current backends do.
>> Indeed, the Boost.Locale rationale clearly
>> says that it only takes std::basic_string
>> and const char* as input because that's what
>> the backend libraries expect anyway.
> Not only, there are more reasons for this:
I can't find any more reasons than what I said.
Am I missing something?
> When Boost.Unicode would be ready I'm sure its users
> would be glad to use its own native interface where
> it is needed.
> So do we need more generic API that would never be used?
Maybe they could want another backend for various reasons.
- ICU is more well-tested
- ICU is more feature-rich
- Boost.Unicode's behaviour is not tailored for particular locales
- Boost.Unicode is written by a crazy frenchman that they do not trust
- ICU may already be installed while Boost.Unicode not
> To be honest I don't think that iterators
> especially over non-UTF charsets are good
> I'd rather accept a concept of stream
> that you read from and write to:
> 1. It is buffered
> 2. It is fast and can work with
> big chunks.
> 3. It can be passed transparently
> and ABI stable to any backends.
> So I would rather see implementation of
> generic algorithms over I/O streams rather
> then iterators.
Ok. I had the feeling that iterators were more popular than streams, but
streams are fine too; they're very close concepts.
> Without it I wouldn't develop the library
> in first place.
I never said to get rid of the stable API, I was just asking if you were
willing to consider giving up the idea of the binaries of the library
backends being swappable at (a potentially runtime) link-time.
i.e. backends would be statically instead of dynamically selected.
But if not, then it's fine, but that does give quite a few limitations
to what the library can do in terms of template stuff.
> What is important that std::codecvt is part
> of C++ standard and thus any library that uses
> this standard would enjor the full power
> of Boost.Locale without even depending on it.
While I agree that support for std::codecvt is important, why not make a
new interface similar to it that doesn't have the same state problems,
and tell users to use that instead when possible?
> I may agree that it is not perfect, but giving
> limitations of current state-of-the-art implementations
> in ICU it is yet reasonable one.
ICU does not have such limitations.
- BreakIterator allows to find boundaries as you iterate the text
- Even if ICU couldn't do that, there is no reason to return in the
operator* of your token_iterator basic_string<ValueType>(begin, end) and
copy the data, you could just return iterator_range<IteratorType>(begin,
Also, while I'm at it, break_iterator really looks like a special case
>> some compile-time values tested at runtime,
>> some things that could have reused existing
>> utilities from Boost,
> More specific points?
I saw stuff like
Additionally, this particular trait is not canonical (should be ::value,
not ::is_linear), is not righly named (it's contiguous, not linear), and
could have other uses outside of Boost.Locale, so it should be in its
>> While most of the backends are separated,
>> it is not the case for encoding/codepage.cpp,
>> which is shared by all backends and contains ifdefs.
>> This is not a very nice extension point.
> Actually not codepage only.
> - Formatting (locale::format)
> - Locale information
> - Message Formatting (gettext)
> - codecvt facets (not details)
> And more...
> So there are lots of shared parts.
I meant shared parts which are not really shared, i.e. a file that is
shared but that contains completely different code chosen using #ifdefs.
>> I'm also a bit concerned about the generalized
>> usage of virtual functions anywhere;
>> it seems unnecessary in quite a few places,
>> please prefer using templates when dynamic binding is not required.
> More specific points?
Couldn't boost::locale::util::base_converter be a concept rather than an
abstract base class?
>> A lot of new and vectors too, I'd prefer if ideally
>> the library never allocated anything.
> I'm sorry but it is just something
> that can't and would never happen.
> This request has no reasonable base especially
> for such complex topic.
Usage of templates instead of inclusion polymorphism would allow to
avoid newing the object and using a smart pointer, for example.
Plus the instances of allocation in the boundary stuff (when you build
an index vector and when you copy into a new basic_string) appears to be
Functions that allocate behind your back are frowned upon, because you
have no mean of controlling how that memory is allocated or managed.
It limits the application range of the library. But then that could be
considered reasonable if the benefits outweigh the drawbacks; I do not
think this is necessarily the case here.
>> I hope my review will be well received
>> and that you will consider all of my "feature requests".
>> Your library is practical and can get things done for sure,
>> but maybe it's not quite as good as it could be in terms of
>> interface yet.
> The interface is a part of the design and have
> designed to be simple and covinent rather then
> fully generic.
Passing an arbitrary range to the functions is convenient, forcing the
user to copy it to a vector and pass the begin and end pointers before
it can call the functions isn't.
See, genericity is convenient.
> It can't be Boost.Locale's backend as at least
> in current state it is locale agnostic
> and misses most important feature localization is actually
> required - CLDR - Common Locale Database Repositry
It would have to be a partial backend (used just for conversion between
UTF encodings, normalization and boundary analysis), and you could fill
the blanks using another backend.