Subject: Re: [boost] [locale] review part 2.1: source
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-04-10 08:25:04
> >> line 150: I think sresult.append(out_start, out_ptr);
> >> would be slightly clearer.
> > I should look on it carefully whether I can change
> > this.
> > iconv API is very gentle in how it handles its pointers
> > and states so I need to do everything carefully.
> > Also I assume that append(Begin,End) may be less
> > efficient then append(pointer,size) as it may be
> > naively implemented as
> > while(begin!=end)
> > s += *begin++;
> Why make such assumptions that could very well be unfounded when it only
> makes your code harder to understand?
I don't think so. I don't think that
is less readable then
But this is the question of programming style.
> >> iconv_between: Inheritance from iconv_from_utf is
> >> a little odd.
> > But it makes a code much simpler.
> Inheritance should not be used for code sharing unless used with the
> CRTP idiom.
I'm sorry but I disagree.
1. Template metaprograming should be used in places where it is justified.
2. If you have a class figure I don't see why shouldn't it have
shared code for circle and square.
This makes sense for me.
I understand that in this particular case the sharing looks odd, but
in fact it makes things only simpler as conv_between is same
as conv from char to to char because iconv supports it
unlike Win32API or ICU API that requires using temporary
> >> iconv_to_utf: The comments from iconv_from_utf apply.
> >> Also, the implementation is almost the same as
> >> iconv_from_utf. I think you could define a single
> >> function template to implement both.
> > Not exactly. I had found that it is much
> > safer and readable to have two implementations
> > then to create quite complex meta programming
> > to make sure it works.
> > It is much easier to assume that at least
> > one side is "char"
> Code duplication is one of the problems with your library in my opinion.
> More on that in my upcoming review.
Generally I try to avoid it, but if you see, you are
welcome to point to such things.
I probably missed some parts or duplicated accidentally
some code during a year and the half of instant development
and evolution of this library.
In any case I would like to ask you to read carefully rationale
behind this library in the tutorial.
I know your Unicode library and I know that you have very
different point of view on how things should be done.
In any case I really looking forward for your review as
the author of Boost.Unicode library.
> So it is intentional that you duplicate code because you find writing it
> once generically makes it hard to validate?
On the second glance it seems to be possible to merge two
In any case... It should be done very-very carefully to make
sure nothing goes wrong.
iconv API is tricky and differs by implementation
so I would have to test it on several platforms
I think I may fix this.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk