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