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

Subject: Re: [boost] [locale] review part 2.1: source
From: Mathias Gaunard (mathias.gaunard_at_[hidden])
Date: 2011-04-10 07:55:12

On 10/04/2011 11:12, Artyom wrote:

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

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

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

So it is intentional that you duplicate code because you find writing it 
once generically makes it hard to validate?