Boost logo

Boost :

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?

Boost list run by bdawes at, gregod at, cpdaniel at, john at