Boost logo

Boost :

Subject: [boost] [locale] Review part 1: headers
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2011-04-08 18:25:46


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.



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.


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.



Again, why the separate specializations?


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.)

  - 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.



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.)


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.


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.


line 62: This assumes that unsigned is 32 bits,
   which implies that you should be using uint32_t.

line 90: Use locale_category_type?


line 79: Why is the argument non-const?


line 29: The constructor should be explicit.

line 36: no need to test for NULL. The compiler
   already checks for NULL for you.


#include <map> and <memory> are unnecessary.


Missing #include <memory> for auto_ptr.

line 129: What about different back ends for different
   char types? Does that even make sense?


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.


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

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