Boost logo

Boost :

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

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, gregod at, cpdaniel at, john at