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


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk