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

Subject: Re: [boost] [locale] Review part 1: headers
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2011-04-09 11:31:03

AMDG

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.

>> line 435: it's somewhat surprising to see
>>    a templated swap.  You should comment that
>>    it requires that the base_iterators must
>>    be the same type.
>>
>
> Good point. I'll add this.
>
>> 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.

>> line 538-539: I don't understand this sentence.
>>    What does "tide" mean?
>>
>
> Doesn't example makes it more clear.
>

Yeah.  It's fairly clear what the class
does.  It was just the use of "tide" that
confused me.  I don't know any meaning of
"tide" which makes any sense in this context,
so I wanted to make sure that it wasn't
just a Unicode specific term that I was
unfamiliar with.

> Probably I use the word "tide" incorrectly,
> basically it means whether I want to
> select everything or I need to select
> strict subsets.
>
> For example if you want to select every
> word or only thous who actually have letters?
>
> So basically it is strict token selection
> should be.
>
>> 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.

>> date_time.hpp:
>>
>> 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.

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

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

>> format.hpp:
>>
>> line 65: Use static_cast instead of reinterpret_cast.
>>    Actually, you don't need a cast at all.  Use static_cast
>>    in write as well.
>>
>
> Noted.
>
>> line 104: shouldn't this be get_position()?
>>
>
> Yes... My bad.
>
>> Is there precedent for this format string syntax?
>
> Actually it is partially based on ICU format, and some
> others formats like Java and so on.
>
> The most important part that this format should
> be rich and extend-able.
>
>>
>> What are the requirements on the encoding of the format string?
>
> 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?

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

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

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

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

In Christ,
Steven Watanabe