AMDG This is getting rather long already, so will be a multi-part review. I've attached a diff of various fixes in comments that I made while going through the headers. locale.hpp: boundary.hpp: line 22: you already #included <iterator> line 47: You don't need to use typedef enum { ... } name; enum name { ... }; is more normal in C++. Is there a reason that you use four bits for each flag? Is this to allow the flags to be split up without breaking binary compatibility? line 102: You assume that unsigned is at least 20 bits. It's usually 32, but I don't think the standard guarantees as much. line 131: Please be consistent about using unsigned vs. uint32_t. line 140: shouldn't offset be std::size_t? Why should the size of the string be limited to 4GB? std::size_t is guaranteed to be large enough to handle anything that can be stored in memory. 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. 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. line 447: assignment is strongly exception safe. you should make this guarantee explititly. line 538-539: I don't understand this sentence. What does "tide" mean? line 532: entry text? I don't think this is right, but I don't know what you meant to say. line 600: token_iterator(mapping_type const &map,bool begin,unsigned mask) Just make this constructor an implementation detail. It's too messy to be part of the public interface. line 628: if(this!=&other) I doubt that this actually helps performance, and the compiler's version of the copy constructor and assignment operator should work fine. line 646: Normally it's undefined behavior to dereference an iterator like this. I would prefer an assert to throwing an exception. break_iterator: most of the comments from token_iterator also apply to break_iterator. collator.hpp line 74: Please describe the meaning of the return value, even if you think it's obvious. line 115: You should spell out the post-conditions of transform more clearly. I presume that the following holds, but I'd like to see it explicitly: Given strings s1 and s2 and collation level level, let t1 = transform(level, s1) and t2 = transform(level, s2). Then, t1 < t2 iff compare(level, s1, s2) < 0. config.hpp: conversion.hpp: Again, why the separate specializations? 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.) operator*: - You missed signed char and long long. - This is one place where a little template metaprogramming would help: template<class T> typename enable_if<is_arithmetic<T>, date_time_period>::type operator*(period::period_type, T); - 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; - the operators need to be defined whenever period_type is defined to avoid accidentally calling the built-in operator if the user forgets to #include a header. line 491: The argument to operator[] should be std::size_t to match std::vector. date_time_facet.hpp: encoding.hpp: lines 103,112,145,154,218: technically data() would be more appropriate than c_str(), since you don't need the null terminator. line 218: Please qualify this call to avoid ADL in namespace std. (Other places in this file too.) 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. line 104: shouldn't this be get_position()? Is there precedent for this format string syntax? What are the requirements on the encoding of the format string? What happens when the format string's syntax is invalid? Undefined behavior? An exception? 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. lines 259-263: I'm sure this conversion from char works in practice, but does the standard guarantee it? lines 341,345: You defined named constants earlier. Why not use them? line 292-293: I'm not sure why you have separate svalue and value. AFAICT, value is used if the value begins with a quote and svalue is used otherwise. formatting.hpp: line 227: string_set::set is not exception safe. if new fails, then you'll be left with the new Char type, the old size, and a null pointer. generator.hpp: line 62: This assumes that unsigned is 32 bits, which implies that you should be using uint32_t. line 90: Use locale_category_type? gnu_gettext.hpp: line 79: Why is the argument non-const? hold_ptr.hpp line 29: The constructor should be explicit. line 36: no need to test for NULL. The compiler already checks for NULL for you. info.hpp: #include <map> and <memory> are unnecessary. localization_backend.hpp: Missing #include <memory> for auto_ptr. line 129: What about different back ends for different char types? Does that even make sense? message.hpp: The compiler generated assignment operator of message only provides basic exception safety. If it fails, the message object may not be meaningful. Please document this or change it. line 776: you could use aggregate initialization. util.hpp line 48: Can you link to the documentation of the format of name? What happens if name is not in the correct format? line 76-77: Please rewrite this sentence. I can't figure out what it's trying to be say. line 108: assert(typeid(*this) == typeid(base_converter))? In Christ, Steven Watanabe