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