Boost logo

Boost :

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

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.


line 311: s/continious/contiguous/

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

line 338: s/collsion/collision/


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


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


line 229: this is not exception-safe


line 145: why not boost::scoped_ptr?


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)


line 239: s/tockenizer/tokenizer/


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


What do //ok comments signify?


lines 82, 85: needs changed to use RAII


line 159: s/impl_std/impl_posix/


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



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

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