Boost logo

Boost :

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


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?


> 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

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