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 14:21:23

> 
> >>  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.
> >
> 
> That's sometimes true.  I know around here
> we tend  to err on the side of too much
> metaprogramming, but in this case I  don't
> think you even need much.

After reviewing the code once again it seems
that you are right. I'll merge it.

> >> lines 86,101: The STL is  your friend:
> >>      size_t  remove_substitutions(std::vector<wchar_t>    &v)
> >>
> >>      {
> >>            v.erase(std::remove(v.begin(), v.end(), wchar_t(0xFFFD)),  
>v.end());
> >>           return  v.size();
> >>      }
> >>
> >
> > After  reading it carefully I can see how it works but
> > generally I see how it  works but it isn't so readable.
> >
> > In any case interesting and it  may save some allocations.
> >
> 
> Well, remove/erase is idiomatic, so it  should be
> clearer to those familiar the STL.
> 

I'm familiar but still it is not trivial to
read and understand complexity, even thou it
so better solution that I'll likely going to use.

> >>  icu/numeric.cpp:
> >>
> >> In  num_format: You don't handle  bool?
> >>
> >
> > ICU does not handle it... So I can't do  something
> > better for this - I pass it to std::locale::classic
> >  formatting.
> >
> 
> Is there any way that you could use the
> message  translation mechanism for "true"
> and "false" if std::boolalpha is  set?
> Otherwise it should be formatted the same
> as an  integer.
> 

In such case I'll have to grab translation dictionaries
with the library and maintain them together with Boost.

It is trivial on Linux like platforms where all dictionaries
usually go to /usr/share/locale/$(LANG)/LC_MESSAGES/boost_locale.mo
but it is less trivial on Windows.

It would be better to give this to translator
that handles all the messages in the software.

So translation of true or false would not add much :-)

In any case it would likely go to some sentence like
"The result of test is true" or something like
that so it would anyway be better to translate
the string in full context as it may be "true" in
Enlish but something context dependent on other
languages.

> >
> >  It is actually fairly simple as streambuf has a buffer
> > so it can  actually receive putback characters without problem.
> >
> 
> For a  limited number of characters.
> If you're reading from a file or from
> a  stringstream, you'll be okay because
> pbackfail can seek to an earlier  position
> in the stream.  If you're reading from
> a terminal, it should  be okay because it's
> usually line buffered, and the lines aren't
> usually  very long.  I could see running
> into a problem if you're reading  from
> a pipe, however.

I think I'll document this limitation.

> 
> > Unfortunately it is the best solution I  could
> > reach so far with the limitation of  std::num_get
> >
> 
> Yeah.  I can't think of anything  better
> either.  This is really more of a limitation
> of std::steambuf,  though.
> 

Yes, indeed.

> 
> >> line 85: If this  fails, don't you still need to call  freelocale?
> >>    I think this would be  simplified if  you created a simple RAII
> >>    wrapper that holds a  locale_t  by value.
> >>
> >
> > Ok... I see missed  this.
> >
> > I prefer to have it shared as every facet has its  own
> > copy (2 characters * 4-5 facets)
> >
> > I'll fix  this.
> 
> Oh, I wasn't suggesting that you go away  from
> shared_ptr:
> 
> struct posix_locale :  boost::noncopyable
> {
>      posix_locale();
>       ~posix_locale();
>      locale_t  impl_;
> };
> 
> shared_ptr<posix_locale> p(new  posix_locale());
> 
> It's harder to get this wrong.

Oh... year, you are right.

Thanks,
  Artyom