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 acm.org, david.abrahams at rcn.com, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk