Re: [boost] [locale] review part 2.1: source

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 

   append(str,size) 

is less readable then 

   append(str_begin,str_end)

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
UTF-16 storage.

> 
> >> 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?
> 

You know...

On the second glance it seems to be possible to merge two
versions.

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
afterward. 

I think I may fix this.

Artyom