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