Re: [boost] [xint] Fifth release -- one more round of reviews, please

Subject: Re: [boost] [xint] Fifth release -- one more round of reviews, please
From: Adam Merz (adammerz_at_[hidden])
Date: 2010-06-13 04:53:21

Chad Nelson <chad.thecomfychair <at> gmail.com> writes:
> Done. There's one new function (to_wstring), everything else should just
> work the way you'd expect it to, whether you're using ASCII strings or
> wide-character ones.
> 
> The changes have just been uploaded to the Sandbox (haven't updated the
> Vault yet). Please take a look and let me know if you see any problems
> with the implementation. There shouldn't be.

Looks good to me -- the compilation errors were solved, which was my only real
concern. A few nitpicks though, since you asked: ;-]

Functions templated on charT taking std::basic_string<charT> should be
templated on charT, traitsT, allocT and take
std::basic_string<charT, traitsT, allocT>.

xint::detail::nan_text should return 'std::basic_string<charT> const&' rather
than just 'std::basic_string<charT>'. This way callers (many integer_t
constructors as well as xint::to_string variants and stream operators) can call
it like 'std::basic_string<charT> const& tnan = detail::nan_text<charT>();'
and avoid making hundreds/thousands of pointless copies of the string.

For functions taking 'charT const*, charT**', this is a bit C-ish; obviously
this is pedantic/stylistic concern, but charT*& is preferred over charT**.

The functions templated on charT are fine, but for the functions specifically
overloaded on std::string/std::wstring (e.g. to_string, to_wstring), consider 
also providing additional overloads for char16_t and char32_t based on
BOOST_NO_CHAR16_T and BOOST_NO_CHAR32_T.

to_string and to_wstring have identical implementations except for the
character type they hardcode in. Refactor these into a function template so
there's a single common implementation, and have outer functions defer to the
template.