[boost] [locale] Review: Design and Implementation

Subject: [boost] [locale] Review: Design and Implementation
From: Gevorg Voskanyan (v_gevorg_at_[hidden])
Date: 2011-04-22 17:49:30

I briefly skimmed over the sources and noticed some things I'd like to get 
addressed. Unfortunately this is far from being a real comprehensive review as I 
would like it to be, due to time constraints.

operator= should return non-const reference.

I don't really like date_time::operator/() as a syntactic sugar for 
date_time::get() when the involved period is not period::year, but e.g. 
period::day_of_week_local. It raises eyebrows when first seeing it, forcing one 
to look at the documentation to find out what that overloaded operator means.

BOOST_LOCALE_DECL needs to be defined in terms of BOOST_SYMBOL_EXPORT and 
BOOST_SYMBOL_IMPORT.
Also the exception classes need to be attributed with BOOST_SYMBOL_VISIBLE (I 
assume all member functions of exception classes are defined inline, which 
appears to be the case).

Many constructors should be explicit but aren't.

Existence of standard C++11 features should be checked with BOOST_NO_XXX macros 
instead of BOOST_HAS_XXX. E.g. !defined BOOST_NO_CHAR16_T  rather than  defined 
BOOST_HAS_CHAR16_T.

Boundary interface should seriously be reconsidered. I would like to see it be 
very much like that of regex_iterator and regex_token_iterator.

Please replace throw with BOOST_THROW_EXCEPTION.

For classes having a swap member function there should also be a free swap 
function found by ADL.

'int unused' parameters for post-in/decrement operators will trigger warnings, 
the parameter name better be removed.

Functions using Boost.Locale facets under the hood (e.g. 
boost::locale::fold_case) should be documented as might throw std::bad_cast.

boundary.hpp:

line 311: s/continious/contiguous/

line 319: e-b might not be well-formed, use std::distance(b, e)

line 338: s/collsion/collision/

date_time.hpp:

line 936: t.time(v); should not be called unless in >> v; succeeded

format.hpp:

line 104: s/get_posision/get_position/   Also, shouldn't this be const?

formatting.hpp:

line 229: this is not exception-safe

localization_backend.hpp:

line 145: why not boost::scoped_ptr?

shared/formatting.cpp:

line 45: this is not exception-safe. Why not use copy-and-swap idiom? (it has no 
swap() currently, but I see no reason why there couldn't be)

shared/mo_lambda.cpp:

line 239: s/tockenizer/tokenizer/

encoding/codepage.cpp:

Why are converter objects allocated on free store and not stack?

icu/all_generator.hpp:

What do //ok  comments signify?

posix/codecvt.cpp:

lines 82, 85: needs changed to use RAII

posix/converter.cpp:

line 159: s/impl_std/impl_posix/

std/std_backend.cpp:

line 229: s/impl icu/impl_std/  (noticed cases like this in some other files as 
well, not necessarily in std/)

Gevorg

P.S.

I plan to submit my final review with vote in 7-8 hours time from now. I hope 
that is OK.