Re: [boost] [locale] Review part 1: headers

Subject: Re: [boost] [locale] Review part 1: headers
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-04-09 14:29:24

> On 04/09/2011 02:50 AM, Artyom wrote:
> >> Why do you have  explicit specializations of
> >> boundary_indexing?  They're all the  same.
> >> The compiler is perfectly capable of instantiating
> >>  them all from a single definition.
> >>
> >
> >
> > Ok this  is something that you'll see all over the Boost.Locale
> > code when it  comes to deal with facets generations.
> >
> > It is in order to support  (brain damaged) DLL platform.
> >
> >  <snip>
> >
> 
> Okay.  You might add a comment to this  effect
> in the code, so no one tries to "fix" it.
> 

Good point. I'll add the rationale.

I think I actually commented it in several
places but not in all ones.

> >>  line 447: assignment is strongly exception safe.
> >>    you  should make this guarantee explititly.
> >>
> >
> > What do you  mean?
> >
> 
> The strong exception guarantee:
> Commit or  rollback.  If the function throws
> an exception then it has no other  effect.
> 

I see, noted.

> >
> >> line 646: Normally it's undefined behavior  to
> >>    dereference an iterator like this.  I  would
> >>    prefer an assert to throwing an  exception.
> >>
> >
> > The point is that these locations are  valid
> > positions just you can't call operator*.
> >
> > I think  it is better behavior because it
> > is quite easy to make a mistake with  this.
> >
> > (From my experience with this)
> >
> 
> Of course,  it's a good thing to detect
> the error.  The thing is that an  exception
> means that you unwind the stack, possibly
> handling the exception  at a higher level.
> That's normally not the correct response
> for a  programming error.
> 

It is similar to std::vector's at()...

Maybe, in any case I had found exception helpful,
especially in situation when you don't a error in some part
to shutdown entire process.

Defensive but forgiving programming.

> >>
> >> line  75: this is dangerous.  These constants
> >>    will be  defined separately in every translation unit
> >>    using  them.  This can cause violations of
> >>    the one  definition rule.  (The standard makes
> >>    a special  exception for integral constants,
> >>    but these are of  class type.)
> >>
> >
> > So how would you suggest to fix it?  Remove static and
> > move their constructors to cpp?
> >
> 
> That's  the safest solution.  It's
> possible to define constants of class
> type  in a header correctly, but it's
> really a pain to do right.  IIRC,
> it  can also interfere with precompiled
> headers on some  compilers.

Noted...

> 
> >> - I think trying to override the built-in enum to  int
> >>    conversion like this is rather fragile.  What  happens
> >>    when someone tries to use an enum  constant:
> >>    enum { this_year = 2011 };
> >>     this_year * period::year;
> >
> >
> > I understand this,  on-the other hand it seems to
> > be very convenient. Because otherwise I'll  have
> > to define for every period class every arithmetic
> > operator  and it would be O(n^2) complexity.
> >
> > I'm not sure if it would be  good either.
> >
> 
> Yeah, there's really no good solution.
> I'd be  inclined to drop the operators
> for period_type altogether, and just
> have  people use the constructor of
> date_time_period.  I know operator*
> is  more convenient and nicer to look at,
> but I'd favor more verbose code  that
> can't possibly go wrong.
> 

I'll think about it, what can be done.

> >> line 218: Please qualify this  call to avoid ADL in
> >>    namespace std.  (Other places  in this file too.)
> >>
> >
> > Can you explain  better?
> >
> 
> Argument dependent lookup is one of the
> most annoying  features of C++.  When you
> make a function call of the form
> f(x, y,  z), the compiler will search
> for f in the current scope and also in
> a set  of extra namespaces determined
> by the types of x, y, and z.  It's  usually
> a good idea to suppress ADL except when
> you explicitly intend to  use it.
> 

I see, noted.

> >
> > ASCII compatible... At least for  '{' and '\'' symbols.
> >
> > This covers all supported and frequently  used
> > encodings in locale context.
> >
> 
> Okay.  One other  thing: How can I put a
> literal '{' in the format  string?

It is something I forget to add to documents.

You should use {{ and }}.

> 
> >>
> >> What happens when the format string's syntax  is
> >> invalid?  Undefined behavior?  An  exception?
> >>
> >
> > If the format is invalid it is  ignored.
> >
> > Rationale: you don't want the translator
> > that  actually provides translated format strings
> > to screw the  program.
> >
> 
> In that case, I hope you have tests
> with various  broken format strings.
> 

AFAIR I have such tests in code, but I'll check this again.

Good point.

> >> What exception guarantee does streaming  provide?
> >> If an exception is thrown, can the stream's  locale
> >> and format flags have changed or are they reset
> >>  correctly to their original values.
> >>
> >
> > Currently they  are not restored.
> >
> > I thought it is better to to try to restore  flags
> > in case of exception as restoring they may
> > actually  trigger other one.
> >
> 
> Okay.  Please document this  behavior.
> 

Noted. I'll add this.

> >> line 776: you could use aggregate  initialization.
> >>
> >
> > What do you mean, message.hpp:776  points to
> >
> > #pragma warning(pop)
> >
> 
> Oops.  That  should be line 766:
>    details::set_domain tmp;
>    tmp.domain_id  = id;
> I was just suggesting:
>    details::set_domain = { id  };
> 

Ok.

> >> util.hpp
> >>
> >> line 108:  assert(typeid(*this) == typeid(base_converter))?
> >>
> >
> >  Why? To make sure it is overloaded?
> >
> 
> Exactly.
> 
> > The  point that clone() is used only in case is_thread_safe() ==  false.
> >
> > I can add this but how really important is  that?
> >
> 
> It isn't very important, but if clone is
> called and it  is not overridden, then
> it is always an error, right? Thus, an
> assertion  is appropriate.
> 

Noted...

Never used this technique, interesting point.

Thanks,
  Artyom